fix(ACS-175): Resolve integrations freeze and improve rate limit handling #839
Conversation
📝 WalkthroughWalkthroughAdds debug logging across Claude profile initialization and terminal resume flows, switches to resolved absolute Claude CLI invocation, implements chunked PTY writes for large payloads, and replaces blocking alerts with localized non-blocking toasts in multiple renderer components. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Main
participant PTY as PTY/Terminal
participant ClaudeCLI as Claude CLI
Renderer->>Main: CLAUDE_PROFILE_INITIALIZE(profileId)
Main->>Main: debug logs, lookup profile, ensure config dir
Main->>PTY: create auth terminal
PTY-->>Main: terminal created (id)
Main->>PTY: wait for terminal ready
Main->>Main: resolve absolute Claude CLI path, build OS-specific loginCommand
Main->>PTY: write loginCommand (chunked if large)
PTY->>ClaudeCLI: command input delivered via terminal
Main->>Renderer: notify terminal-created(profileId, terminalId)
Renderer->>Renderer: show localized toast (authenticating)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)apps/frontend/src/shared/i18n/locales/**/*.json📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-12-30T16:38:36.314ZApplied to files:
📚 Learning: 2026-01-10T15:30:12.808ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @MikeeBuilds, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience by addressing UI freezing issues during Claude profile integration and improving how rate limit modals are handled. It achieves this by implementing a chunked writing mechanism for terminal processes to prevent UI blocking, replacing intrusive Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully resolves a UI freeze issue by implementing a chunked writing mechanism for large data payloads and improves the user experience by replacing blocking alert() dialogs with non-blocking toast notifications. The changes are well-targeted. However, a significant amount of debugging console.log statements have been introduced across multiple files, which must be removed before merging. Additionally, there are opportunities to improve error handling consistency and code clarity in the new PTY writing logic.
| ipcMain.handle( | ||
| IPC_CHANNELS.CLAUDE_PROFILE_INITIALIZE, | ||
| async (_, profileId: string): Promise<IPCResult> => { | ||
| console.log('[IPC:CLAUDE_PROFILE_INITIALIZE] Handler called for profileId:', profileId); |
There was a problem hiding this comment.
A large number of console.log and console.error statements have been added throughout this file for debugging (e.g., here and on lines 317, 319, 322, 325, 341, 350, 352, 363, 365, 369, 372, 396, 397, 401, 403, 407, 417, 426). These should be removed before merging to keep production logs clean. If some logging is desired, please use the existing debugLog and debugError utilities.
| */ | ||
| export function writeToPty(terminal: TerminalProcess, data: string): void { | ||
| terminal.pty.write(data); | ||
| console.log('[PtyManager:writeToPty] About to write to pty, data length:', data.length); |
There was a problem hiding this comment.
This function contains many console.log and console.error statements for debugging. Please remove them all before merging to avoid polluting production logs. If logging is necessary, consider using a dedicated, level-based logger like debugLog and debugError which are used elsewhere in the codebase.
| console.error('[PtyManager:writeToPty] Write FAILED:', error); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
The error handling here is inconsistent with the chunked write logic. While the chunked path catches and logs errors, this path re-throws the error. Since the caller (TerminalManager.write) doesn't handle exceptions, this could lead to an uncaught exception. For consistency and stability, you should log the error here using debugError instead of re-throwing it. This will require importing debugError from ../../shared/utils/debug-logger.
} catch (error) {
console.error('[PtyManager:writeToPty] Write FAILED:', error);
}| * Send input to a terminal | ||
| */ | ||
| write(id: string, data: string): void { | ||
| console.log('[TerminalManager:write] Writing to terminal:', id, 'data length:', data.length); |
There was a problem hiding this comment.
These console.log and console.error statements (lines 123, 126, 128, 130) appear to be for debugging. Please remove them before merging to keep production logs clean. If logging is needed, consider using the debugLog and debugError utilities available in the project.
| }; | ||
|
|
||
| const handleAuthenticateProfile = async (profileId: string) => { | ||
| console.log('[IntegrationSettings] handleAuthenticateProfile called for:', profileId); |
There was a problem hiding this comment.
This component contains several console.log statements (lines 215, 218, 220, 231, 238) that seem to be for debugging. Please remove them before merging.
| if (data.length > 1000) { | ||
| console.log('[PtyManager:writeToPty] Large write detected, using chunked write'); | ||
| const chunkSize = 100; // Smaller chunks |
There was a problem hiding this comment.
The values 1000 and 100 are magic numbers. Please extract them into named constants at the top of the file for improved readability and maintainability, for example:
const LARGE_WRITE_THRESHOLD = 1000;
const WRITE_CHUNK_SIZE = 100;Then use these constants in the code.
MikeeBuilds
added
priority/high
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:
- Line 30: OAuthStep currently logs toast messages with hardcoded English
strings; import and call useTranslation (e.g., const { t } =
useTranslation('settings')) at the top of the OAuthStep component and replace
every hardcoded toast message in functions that call useToast (locations around
the Toast calls currently at OAuth success, auth start failed, add profile
failed, auth failed, token saved success, token save failed and the token save
catch) with the appropriate translation keys from settings.json (use
t('namespace.key') or t('oauth.success') style keys matching the entries added
in settings.json) while keeping the existing useToast invocation and payload
structure unchanged so only the message/title fields use t(...) instead of
literal strings.
In @apps/frontend/src/renderer/components/RateLimitModal.tsx:
- Around line 136-140: The toast call in RateLimitModal (the toast invocation
with variant: 'destructive', title: 'Failed to add profile', description:
'Please try again.') uses hardcoded strings; replace them with i18n translation
keys following the pattern used in IntegrationSettings.tsx (use the project's
translation hook/utility and keys like
integrationSettings.addProfileFailed.title and .description or similar),
ensuring you call the translation function inside RateLimitModal and pass the
translated strings to toast instead of literal text.
In @apps/frontend/src/renderer/components/SDKRateLimitModal.tsx:
- Around line 180-184: The toast call in SDKRateLimitModal uses hardcoded
English strings; replace them with i18n translation keys by using the existing
translation hook (e.g., the component's useTranslation/useI18n -> t) and call
toast({ variant: 'destructive', title:
t('sdkRateLimitModal.addProfileFailed.title'), description:
t('sdkRateLimitModal.addProfileFailed.description') }); add corresponding keys
to the locale JSON files following the project's naming convention
(sdkRateLimitModal.addProfileFailed.title and .description) and ensure the
translation hook is imported/initialized in the SDKRateLimitModal component if
not already.
- Around line 165-175: The toast calls in SDKRateLimitModal.tsx use hardcoded
English strings; import and call the i18n hook (useTranslation) in the
component, replace the literal titles/descriptions with translation keys (e.g.,
t('sdk.authenticating', { profileName }) and t('sdk.check_terminal') for the
success toast, and t('sdk.auth_failed') with either initResult.error or
t('sdk.try_again') for the error toast), and ensure profileName and
initResult.error are passed into the translation/interpolation where needed.
In @apps/frontend/src/renderer/components/settings/IntegrationSettings.tsx:
- Line 215: Replace the ad-hoc console.log calls in handleAuthenticateProfile
with the project's debug logging utility: remove the console.log statements
(including the ones at lines shown) and call debugLog(...) instead, importing
debugLog from the same utility module used elsewhere; ensure each message
preserves its context (e.g., include profileId and relevant state) and matches
existing debugLog usage patterns in other components so logs are consistent and
can be toggled in production.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
apps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/renderer/components/settings/IntegrationSettings.tsxapps/frontend/src/shared/i18n/locales/en/settings.jsonapps/frontend/src/shared/i18n/locales/fr/settings.json
🧰 Additional context used
📓 Path-based instructions (4)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/settings/IntegrationSettings.tsx
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/settings/IntegrationSettings.tsx
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/settings/IntegrationSettings.tsx
apps/frontend/src/shared/i18n/locales/**/*.json
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/shared/i18n/locales/**/*.json: Store translation strings in namespace-organized JSON files atapps/frontend/src/shared/i18n/locales/{lang}/*.jsonfor each supported language
When implementing new frontend features, add translation keys to all language files (minimum: en/.json and fr/.json)
Files:
apps/frontend/src/shared/i18n/locales/en/settings.jsonapps/frontend/src/shared/i18n/locales/fr/settings.json
🧠 Learnings (3)
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/settings/IntegrationSettings.tsx
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/frontend/src/shared/i18n/locales/**/*.json : When implementing new frontend features, add translation keys to all language files (minimum: en/*.json and fr/*.json)
Applied to files:
apps/frontend/src/shared/i18n/locales/en/settings.jsonapps/frontend/src/shared/i18n/locales/fr/settings.json
📚 Learning: 2026-01-04T12:37:52.222Z
Learnt from: StillKnotKnown
Repo: AndyMik90/Auto-Claude PR: 648
File: apps/frontend/src/renderer/components/settings/ModelSearchableSelect.tsx:262-265
Timestamp: 2026-01-04T12:37:52.222Z
Learning: Dropdown components in apps/frontend/src/renderer/components/settings use a fixed max-height (e.g., max-h-60) with internal overflow-y-auto scrolling. This pattern is intentional and prevents dropdowns from extending beyond the viewport; do not add dynamic viewport positioning or clipping-prevention logic for these components. If you add new dropdowns in this area, follow the same fixed-height + internal-scroll approach for consistency.
Applied to files:
apps/frontend/src/renderer/components/settings/IntegrationSettings.tsx
🧬 Code graph analysis (5)
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx (1)
apps/frontend/src/renderer/hooks/use-toast.ts (2)
useToast(192-192)toast(192-192)
apps/frontend/src/main/terminal/pty-manager.ts (1)
apps/frontend/src/main/terminal/types.ts (1)
TerminalProcess(8-22)
apps/frontend/src/main/ipc-handlers/terminal-handlers.ts (3)
apps/frontend/src/main/claude-profile-manager.ts (1)
getClaudeProfileManager(560-565)apps/frontend/src/main/claude-cli-utils.ts (1)
getClaudeCliInvocationAsync(66-77)apps/frontend/src/shared/utils/shell-escape.ts (1)
escapeShellArg(24-29)
apps/frontend/src/renderer/components/RateLimitModal.tsx (1)
apps/frontend/src/renderer/hooks/use-toast.ts (2)
useToast(192-192)toast(192-192)
apps/frontend/src/renderer/components/settings/IntegrationSettings.tsx (1)
apps/frontend/src/renderer/hooks/use-toast.ts (2)
useToast(192-192)toast(192-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
apps/frontend/src/main/terminal/terminal-manager.ts (1)
123-131: LGTM! Diagnostic logging aids troubleshooting.The added logging provides visibility into terminal write operations, which aligns with the PR's goal of resolving integration freeze issues. The logs track data length, terminal existence, and operation completion.
apps/frontend/src/shared/i18n/locales/en/settings.json (1)
440-451: LGTM! Translation keys properly added.The new toast notification keys are well-structured and provide clear user-facing messages for authentication, profile management, and token handling scenarios.
Note: Ensure these translation keys are referenced in UI components rather than hardcoded strings (see OAuthStep.tsx review).
apps/frontend/src/shared/i18n/locales/fr/settings.json (1)
440-451: LGTM! French translations properly added.The toast keys mirror the English locale structure, maintaining consistency across languages as required by the coding guidelines.
Based on learnings, both en/.json and fr/.json must be updated for new features.
apps/frontend/src/renderer/components/RateLimitModal.tsx (1)
25-25: LGTM!The useToast hook is correctly imported and initialized.
Also applies to: 33-33
apps/frontend/src/renderer/components/SDKRateLimitModal.tsx (1)
24-24: LGTM!The useToast hook is correctly imported and initialized.
Also applies to: 59-59
apps/frontend/src/renderer/components/settings/IntegrationSettings.tsx (2)
32-32: LGTM!The useToast hook is correctly imported and initialized.
Also applies to: 47-47
88-92: Excellent i18n usage in toast notifications!The toast notifications correctly use translation keys via the
t()function, and the useEffect dependencies properly include bothtandtoast. This is the correct pattern that should be followed throughout the codebase.Also applies to: 97-97
apps/frontend/src/main/ipc-handlers/terminal-handlers.ts (1)
368-394: LGTM - Secure command construction with proper shell escaping.The command construction has been refactored to use the full Claude CLI path directly, with proper shell escaping for both Windows and POSIX systems. This approach is more secure than PATH manipulation and correctly handles:
- Windows cmd.exe syntax with
escapeShellArgWindows- Unix/Mac bash/zsh syntax with
escapeShellArg- Environment variable setting for non-default profiles
- Simple command for default profiles
| export function writeToPty(terminal: TerminalProcess, data: string): void { | ||
| terminal.pty.write(data); | ||
| console.log('[PtyManager:writeToPty] About to write to pty, data length:', data.length); | ||
|
|
||
| // For large commands, write in chunks to prevent blocking | ||
| if (data.length > 1000) { | ||
| console.log('[PtyManager:writeToPty] Large write detected, using chunked write'); | ||
| const chunkSize = 100; // Smaller chunks | ||
| let offset = 0; | ||
| let chunkNum = 0; | ||
|
|
||
| const writeChunk = () => { | ||
| if (offset >= data.length) { | ||
| console.log('[PtyManager:writeToPty] Chunked write completed, total chunks:', chunkNum); | ||
| return; | ||
| } | ||
|
|
||
| const chunk = data.slice(offset, offset + chunkSize); | ||
| chunkNum++; | ||
| console.log('[PtyManager:writeToPty] Writing chunk', chunkNum, 'offset:', offset, 'size:', chunk.length); | ||
| try { | ||
| terminal.pty.write(chunk); | ||
| console.log('[PtyManager:writeToPty] Chunk', chunkNum, 'written'); | ||
| offset += chunkSize; | ||
| // Use setImmediate to yield to the event loop between chunks | ||
| setImmediate(writeChunk); | ||
| } catch (error) { | ||
| console.error('[PtyManager:writeToPty] Chunked write FAILED at chunk', chunkNum, 'offset', offset, ':', error); | ||
| } | ||
| }; | ||
|
|
||
| // Start the chunked write after yielding | ||
| console.log('[PtyManager:writeToPty] Scheduling first chunk...'); | ||
| setImmediate(writeChunk); | ||
| console.log('[PtyManager:writeToPty] First chunk scheduled, returning'); | ||
| } else { | ||
| try { | ||
| terminal.pty.write(data); | ||
| console.log('[PtyManager:writeToPty] Write completed successfully'); | ||
| } catch (error) { | ||
| console.error('[PtyManager:writeToPty] Write FAILED:', error); | ||
| throw error; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Asynchronous chunked write with no completion tracking.
The function returns immediately for large writes while chunks continue writing asynchronously via setImmediate. This creates several issues:
- No completion notification – Callers cannot determine when the write finishes.
- Error handling gap – Errors in later chunks (line 184) are logged but not propagated to the caller since the function has already returned.
- Race condition risk – The terminal PTY could be killed or the
TerminalProcessobject destroyed while chunks are still queued. - Very small chunks – 100-byte chunks for data >1000 bytes could queue hundreds of callbacks for large payloads.
Recommended fixes
Option 1: Return a Promise so callers can await completion:
-export function writeToPty(terminal: TerminalProcess, data: string): void {
+export function writeToPty(terminal: TerminalProcess, data: string): Promise<void> {
console.log('[PtyManager:writeToPty] About to write to pty, data length:', data.length);
// For large commands, write in chunks to prevent blocking
if (data.length > 1000) {
console.log('[PtyManager:writeToPty] Large write detected, using chunked write');
- const chunkSize = 100; // Smaller chunks
+ const chunkSize = 1000; // Larger chunks to reduce overhead
let offset = 0;
let chunkNum = 0;
- const writeChunk = () => {
+ return new Promise<void>((resolve, reject) => {
+ const writeChunk = () => {
- if (offset >= data.length) {
- console.log('[PtyManager:writeToPty] Chunked write completed, total chunks:', chunkNum);
- return;
- }
+ if (offset >= data.length) {
+ console.log('[PtyManager:writeToPty] Chunked write completed, total chunks:', chunkNum);
+ resolve();
+ return;
+ }
- const chunk = data.slice(offset, offset + chunkSize);
- chunkNum++;
- console.log('[PtyManager:writeToPty] Writing chunk', chunkNum, 'offset:', offset, 'size:', chunk.length);
- try {
- terminal.pty.write(chunk);
- console.log('[PtyManager:writeToPty] Chunk', chunkNum, 'written');
- offset += chunkSize;
- // Use setImmediate to yield to the event loop between chunks
- setImmediate(writeChunk);
- } catch (error) {
- console.error('[PtyManager:writeToPty] Chunked write FAILED at chunk', chunkNum, 'offset', offset, ':', error);
- }
- };
+ const chunk = data.slice(offset, offset + chunkSize);
+ chunkNum++;
+ console.log('[PtyManager:writeToPty] Writing chunk', chunkNum, 'offset:', offset, 'size:', chunk.length);
+ try {
+ terminal.pty.write(chunk);
+ console.log('[PtyManager:writeToPty] Chunk', chunkNum, 'written');
+ offset += chunkSize;
+ setImmediate(writeChunk);
+ } catch (error) {
+ console.error('[PtyManager:writeToPty] Chunked write FAILED at chunk', chunkNum, 'offset', offset, ':', error);
+ reject(error);
+ }
+ };
- // Start the chunked write after yielding
- console.log('[PtyManager:writeToPty] Scheduling first chunk...');
- setImmediate(writeChunk);
- console.log('[PtyManager:writeToPty] First chunk scheduled, returning');
+ console.log('[PtyManager:writeToPty] Scheduling first chunk...');
+ setImmediate(writeChunk);
+ });
} else {
try {
terminal.pty.write(data);
console.log('[PtyManager:writeToPty] Write completed successfully');
+ return Promise.resolve();
} catch (error) {
console.error('[PtyManager:writeToPty] Write FAILED:', error);
- throw error;
+ return Promise.reject(error);
}
}
}Then update the caller in terminal-manager.ts:
- write(id: string, data: string): void {
+ async write(id: string, data: string): Promise<void> {
console.log('[TerminalManager:write] Writing to terminal:', id, 'data length:', data.length);
const terminal = this.terminals.get(id);
if (terminal) {
console.log('[TerminalManager:write] Terminal found, calling writeToPty...');
- PtyManager.writeToPty(terminal, data);
+ await PtyManager.writeToPty(terminal, data);
console.log('[TerminalManager:write] writeToPty completed');
} else {
console.error('[TerminalManager:write] Terminal NOT found:', id);
}
}Option 2: If you cannot change the signature, at least track pending writes and handle terminal destruction:
+// Track pending writes per terminal
+const pendingWrites = new Map<string, number>();
+
export function writeToPty(terminal: TerminalProcess, data: string): void {
console.log('[PtyManager:writeToPty] About to write to pty, data length:', data.length);
if (data.length > 1000) {
+ const terminalId = terminal.id;
+ pendingWrites.set(terminalId, (pendingWrites.get(terminalId) || 0) + 1);
+
const writeChunk = () => {
+ // Check if terminal still exists
+ if (!terminal.pty) {
+ console.warn('[PtyManager:writeToPty] Terminal destroyed, aborting chunked write');
+ pendingWrites.set(terminalId, Math.max(0, (pendingWrites.get(terminalId) || 1) - 1));
+ return;
+ }
+
if (offset >= data.length) {
console.log('[PtyManager:writeToPty] Chunked write completed');
+ pendingWrites.set(terminalId, Math.max(0, (pendingWrites.get(terminalId) || 1) - 1));
return;
}
// ... rest of chunk logic
};
}
}Committable suggestion skipped: line range outside the PR's diff.
apps/frontend/src/renderer/components/settings/IntegrationSettings.tsx
Outdated
Show resolved
Hide resolved
Addresses CodeRabbit review feedback on PR AndyMik90#839: - OAuthStep.tsx: use t() for all toast messages - RateLimitModal.tsx: use t() for toast messages - SDKRateLimitModal.tsx: add useTranslation hook, use t() for toasts - Add toast translation keys to en/fr onboarding.json and common.json
|
@coderabbitai @gemini-code-assist I've addressed the i18n feedback:
Please re-review when convenient! 🙏 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4257 @MikeeBuilds Perfect! I've verified all your i18n changes and they look excellent! ✅ Verification Results: ✅ OAuthStep.tsx: All toast calls now use Your implementation correctly uses the proper namespace prefixes and includes complete translations in both English and French. The i18n feedback has been fully addressed! 🎉 🧠 Learnings used |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves UI freezes by replacing blocking alert dialogs with non-blocking toast notifications and optimizing terminal data writing. The introduction of chunked writing in pty-manager.ts is a great performance improvement for handling large data outputs. The consistent use of toast notifications across different components improves the user experience significantly.
My main feedback is regarding the numerous console.log and console.error statements added for debugging purposes. While useful during development, they should be removed before merging to keep the production console clean. I've left comments on the relevant files to point these out.
| console.log('[PtyManager:writeToPty] About to write to pty, data length:', data.length); | ||
|
|
||
| // For large commands, write in chunks to prevent blocking | ||
| if (data.length > 1000) { | ||
| console.log('[PtyManager:writeToPty] Large write detected, using chunked write'); | ||
| const chunkSize = 100; // Smaller chunks | ||
| let offset = 0; | ||
| let chunkNum = 0; | ||
|
|
||
| const writeChunk = () => { | ||
| if (offset >= data.length) { | ||
| console.log('[PtyManager:writeToPty] Chunked write completed, total chunks:', chunkNum); | ||
| return; | ||
| } | ||
|
|
||
| const chunk = data.slice(offset, offset + chunkSize); | ||
| chunkNum++; | ||
| console.log('[PtyManager:writeToPty] Writing chunk', chunkNum, 'offset:', offset, 'size:', chunk.length); | ||
| try { | ||
| terminal.pty.write(chunk); | ||
| console.log('[PtyManager:writeToPty] Chunk', chunkNum, 'written'); | ||
| offset += chunkSize; | ||
| // Use setImmediate to yield to the event loop between chunks | ||
| setImmediate(writeChunk); | ||
| } catch (error) { | ||
| console.error('[PtyManager:writeToPty] Chunked write FAILED at chunk', chunkNum, 'offset', offset, ':', error); | ||
| } | ||
| }; | ||
|
|
||
| // Start the chunked write after yielding | ||
| console.log('[PtyManager:writeToPty] Scheduling first chunk...'); | ||
| setImmediate(writeChunk); | ||
| console.log('[PtyManager:writeToPty] First chunk scheduled, returning'); | ||
| } else { | ||
| try { | ||
| terminal.pty.write(data); | ||
| console.log('[PtyManager:writeToPty] Write completed successfully'); | ||
| } catch (error) { | ||
| console.error('[PtyManager:writeToPty] Write FAILED:', error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
This function contains a lot of console.log and console.error statements for debugging the chunked writing logic. Please remove them before merging.
| console.log('[TerminalManager:write] Writing to terminal:', id, 'data length:', data.length); | ||
| const terminal = this.terminals.get(id); | ||
| if (terminal) { | ||
| console.log('[TerminalManager:write] Terminal found, calling writeToPty...'); | ||
| PtyManager.writeToPty(terminal, data); | ||
| console.log('[TerminalManager:write] writeToPty completed'); | ||
| } else { | ||
| console.error('[TerminalManager:write] Terminal NOT found:', id); | ||
| } |
There was a problem hiding this comment.
These console.log and console.error statements seem to be for debugging and should be removed before this is merged.
| console.log('[IntegrationSettings] handleAuthenticateProfile called for:', profileId); | ||
| setAuthenticatingProfileId(profileId); | ||
| try { | ||
| console.log('[IntegrationSettings] Calling initializeClaudeProfile IPC...'); | ||
| const initResult = await window.electronAPI.initializeClaudeProfile(profileId); | ||
| console.log('[IntegrationSettings] IPC returned:', initResult); | ||
| if (!initResult.success) { | ||
| alert(`Failed to start authentication: ${initResult.error || 'Please try again.'}`); | ||
| toast({ | ||
| variant: 'destructive', | ||
| title: t('integrations.toast.authStartFailed'), | ||
| description: initResult.error || t('integrations.toast.tryAgain'), | ||
| }); | ||
| } | ||
| // Note: If successful, the terminal is now visible in the UI via the onTerminalAuthCreated event | ||
| // Users can see the 'claude setup-token' output and complete OAuth flow directly | ||
| } catch (err) { | ||
| console.error('Failed to authenticate profile:', err); | ||
| alert('Failed to start authentication. Please try again.'); | ||
| console.error('[IntegrationSettings] Failed to authenticate profile:', err); | ||
| toast({ | ||
| variant: 'destructive', | ||
| title: t('integrations.toast.authStartFailed'), | ||
| description: t('integrations.toast.tryAgain'), | ||
| }); | ||
| } finally { | ||
| console.log('[IntegrationSettings] finally block - clearing authenticatingProfileId'); | ||
| setAuthenticatingProfileId(null); |
There was a problem hiding this comment.
The console.log and console.error statements in handleAuthenticateProfile were likely added for debugging and should be removed.
Addresses CodeRabbit review feedback - use project's debug logging utility for consistent and toggle-able logging in production.
|
@coderabbitai @gemini-code-assist @AndyMik90 Pushed an additional fix for the
All CodeRabbit review items should now be addressed. Ready for re-review! 🙏 |
|
Hello @MikeeBuilds, thank you for the update! I've noted the additional fix for replacing This is a good improvement for consistent and controlled logging within the application. I'll proceed with a re-review of the updated changes. 🙏 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 940 @MikeeBuilds Excellent work! I've verified all your Verification Results: ✅ debugLog import added: Line 33 shows Your implementation is clean and follows the project's logging standards. All review feedback has been fully addressed! 🎉 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
| let offset = 0; | ||
| let chunkNum = 0; | ||
|
|
||
| const writeChunk = () => { |
There was a problem hiding this comment.
@MikeeBuilds @AndyMik90 I think we are good on this for a while, but we may want to think ahead to see what something like Tauri would look like for a Cloud version. Much better multi-threading with Rust that would allow us to avoid some of these blocking scenarios.
aslaker
left a comment
aslaker
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 9 issue(s) require attention.
9 issue(s) must be addressed (5 required, 4 recommended), 1 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Medium | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- High: 5 issue(s)
- Medium: 4 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (10 selected of 10 total)
🟠 [5e70bef47e08] [HIGH] Console.log statements not replaced with debugLog as claimed
📁 apps/frontend/src/main/ipc-handlers/terminal-handlers.ts:315
The PR description claims 'Replaced console.log with debugLog' but this handler contains 14+ console.log statements (lines 315, 317, 319, 322, 325, 341, 350, 352, 363, 365, 369, 372, 396, 397, 401, 403, 407, 417). This same file already imports and uses debugLog elsewhere (lines 10, 187-286, 332, 342, 398), showing the pattern is established but not followed in the new code.
Suggested fix:
Replace all console.log statements with debugLog, and console.error with debugError. The imports already exist in this file.
🟠 [246d2cf4bf39] [HIGH] Verbose console.log statements in writeToPty function
📁 apps/frontend/src/main/terminal/pty-manager.ts:159
The writeToPty function contains 10 console.log/console.error statements that log on every PTY write operation. This will generate excessive log output during normal operation. The codebase pattern for main process files is to use debugLog from the shared debug-logger utility.
Suggested fix:
Import debugLog from '../../shared/utils/debug-logger' and replace console.log statements. For console.error, use debugError.
🟡 [a806f5211b8d] [MEDIUM] Console.log statements in write method
📁 apps/frontend/src/main/terminal/terminal-manager.ts:123
The write method contains 4 console.log/console.error statements that log on every terminal write operation. Should use debugLog for consistency with codebase patterns.
Suggested fix:
Replace console.log on lines 123, 126, 128 with debugLog. The console.error on line 130 could stay or use debugError.
🟠 [e26db54eba18] [HIGH] Hardcoded UI strings violate i18n guidelines
📁 apps/frontend/src/renderer/components/SDKRateLimitModal.tsx:247
This component has numerous hardcoded English strings instead of using i18n translation keys. Examples: 'Claude Code Rate Limit' (line 247), 'Upgrade to Pro for Higher Limits', 'Select account...', 'Switch Account & Retry', 'Retrying...', 'Retry', 'Close'. The sibling RateLimitModal.tsx correctly uses t('rateLimit.modalTitle') patterns.
Suggested fix:
Replace all hardcoded strings with translation keys using t() function. Add corresponding keys to en/common.json and fr/common.json under the rateLimit section.
🟠 [706370be0a87] [HIGH] Hardcoded UI strings violate i18n guidelines
📁 apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:328
This component has many hardcoded English strings despite importing useTranslation: 'Configure Claude Authentication', 'Add your Claude accounts to enable AI features', 'No accounts configured yet', 'Default', 'Active', 'Authenticated', 'Needs Auth', 'Authenticate', 'Set Active', 'Manual Token Entry'. The nearly identical IntegrationSettings.tsx correctly uses translation keys.
Suggested fix:
Replace hardcoded strings with translation keys. Either reuse existing keys from 'settings:integrations' namespace or add new keys to onboarding.json under an 'oauth' section.
🟠 [da5694d192f2] [HIGH] Error handling inconsistency between chunked and non-chunked writes
📁 apps/frontend/src/main/terminal/pty-manager.ts:183
When an error occurs during a chunked write (line 183-185), the error is caught and logged but NOT re-thrown - the function silently stops. However, when an error occurs during a non-chunked write (line 196-199), the error is caught, logged, AND re-thrown. This inconsistency means callers cannot reliably detect write failures for large data.
Suggested fix:
Make behavior consistent: either don't re-throw in non-chunked path (match chunked behavior), or add callback mechanism to propagate chunked errors. Given fire-and-forget terminal semantics, option 1 (silent logging for both) is likely acceptable.
🟡 [269da98a9987] [MEDIUM] Race condition: terminal destruction during chunked write
📁 apps/frontend/src/main/terminal/pty-manager.ts:190
The function returns immediately via setImmediate(writeChunk) while chunks continue asynchronously. If the terminal is destroyed mid-write, subsequent chunk writes attempt to write to a killed PTY process. The error is caught but this represents undefined behavior with silent partial writes.
Suggested fix:
Add a check at the start of writeChunk to verify terminal.pty is still valid before attempting to write. Could check if terminal is still in the terminals Map, or add an 'isDestroyed' flag to TerminalProcess.
🟡 [43d2e30dfd5c] [MEDIUM] Race condition: rapid successive calls can interleave chunks
📁 apps/frontend/src/main/terminal/pty-manager.ts:190
When writeToPty is called multiple times rapidly with data > 1000 bytes, chunks from different calls can interleave because setImmediate callbacks are queued in FIFO order. For example, two calls with 'AAAA...' and 'BBBB...' could result in 'AABBAA...' being written to the PTY.
Suggested fix:
Implement a write queue at the terminal level that ensures one large write completes before the next begins. Or use a mutex pattern to serialize chunked writes per terminal.
🔵 [af9d6c22ded3] [LOW] Magic numbers in chunked write logic
📁 apps/frontend/src/main/terminal/pty-manager.ts:162
The writeToPty function uses magic numbers 1000 (threshold for chunked writing) and 100 (chunk size) without explanation. These values affect performance and behavior but are not documented or made configurable.
Suggested fix:
Extract to named constants: const CHUNKED_WRITE_THRESHOLD = 1000; const CHUNK_SIZE = 100; Add a comment explaining why these values were chosen.
🟡 [bb3f5317951a] [MEDIUM] Login command logged to console in production
📁 apps/frontend/src/main/ipc-handlers/terminal-handlers.ts:397
Line 397 logs the complete login command to console: 'Login command: '. While this specific command may not contain secrets, logging full commands to console (not debugLog) means this appears in production logs.
Suggested fix:
Replace with debugLog so it only appears when DEBUG=true. Consider whether logging the full command is necessary.
This review was generated by Auto Claude.
Addresses Auto-Claude PR Review feedback: - terminal-handlers.ts: 14 console.log → debugLog - pty-manager.ts: 10 console.log → debugLog/debugError - terminal-manager.ts: 4 console.log → debugLog/debugError Also fixes: - Extract magic numbers to CHUNKED_WRITE_THRESHOLD/CHUNK_SIZE constants - Add terminal validity check before chunked writes - Consistent error handling (no rethrow for fire-and-forget semantics)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/ipc-handlers/terminal-handlers.ts (1)
425-432: Remove redundant error logging.Lines 426-427 log the same exception twice: once with
console.errorand once withdebugError. This is redundant and inconsistent with the diagnostic logging approach used throughout the rest of the handler.♻️ Remove the redundant console.error call
} catch (error) { - console.error('[IPC:CLAUDE_PROFILE_INITIALIZE] EXCEPTION:', error); debugError('[IPC] Failed to initialize Claude profile:', error); return { success: false,
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/pty-manager.ts:
- Around line 176-201: The per-chunk debug noise comes from the two debugLog
calls inside writeChunk (the "Writing chunk" and "Chunk written" logs); reduce
verbosity by removing those per-chunk debugLog calls and instead emit a single
debugLog when the chunked write starts (before first setImmediate loop), retain
the existing completion log (when offset >= data.length) and keep debugError on
catch; update references inside writeChunk, CHUNK_SIZE, terminal.pty and
setImmediate accordingly so behavior stays the same but without per-chunk
logging.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/main/terminal/terminal-manager.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/main/terminal/pty-manager.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/main/terminal/pty-manager.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/main/terminal/pty-manager.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2025-12-22T22:43:58.052Z
Learnt from: AndyMik90
Repo: AndyMik90/Auto-Claude PR: 150
File: apps/frontend/src/renderer/components/settings/ProjectSettingsContent.tsx:0-0
Timestamp: 2025-12-22T22:43:58.052Z
Learning: In the AndyMik90/Auto-Claude repository, the ESLint configuration only allows `console.warn` and `console.error` levels, not `console.log` or `console.info`.
Applied to files:
apps/frontend/src/main/terminal/terminal-manager.ts
🧬 Code graph analysis (3)
apps/frontend/src/main/ipc-handlers/terminal-handlers.ts (3)
apps/frontend/src/main/claude-profile-manager.ts (1)
getClaudeProfileManager(560-565)apps/frontend/src/main/claude-cli-utils.ts (1)
getClaudeCliInvocationAsync(66-77)apps/frontend/src/shared/utils/shell-escape.ts (1)
escapeShellArg(24-29)
apps/frontend/src/main/terminal/terminal-manager.ts (1)
apps/frontend/src/shared/utils/debug-logger.ts (1)
debugError(25-29)
apps/frontend/src/main/terminal/pty-manager.ts (2)
apps/frontend/src/main/terminal/types.ts (1)
TerminalProcess(8-22)apps/frontend/src/shared/utils/debug-logger.ts (1)
debugError(25-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
apps/frontend/src/main/terminal/terminal-manager.ts (1)
20-20: LGTM! Debug logging additions improve diagnostics.The added
debugLoganddebugErrorcalls provide useful visibility into terminal write operations without impacting production performance (since they're conditional on debug being enabled). The explicit handling of the "terminal not found" case withdebugErroris a good improvement over the previous silent no-op.Also applies to: 123-133
apps/frontend/src/main/terminal/pty-manager.ts (2)
155-161: LGTM! Constants for chunked write are well-documented.The threshold (1000 bytes) and chunk size (100 bytes) are reasonable for preventing event-loop blocking. The documentation clearly explains the purpose of each constant.
207-215: LGTM! Non-chunked write path with consistent error handling.The error handling correctly matches the chunked path's fire-and-forget semantics, ensuring consistent behavior regardless of data size.
apps/frontend/src/main/ipc-handlers/terminal-handlers.ts (5)
315-325: LGTM: Enhanced diagnostics for profile retrieval.The debug logging added here provides clear visibility into the profile lookup flow, which directly supports the PR objective of troubleshooting integration freeze issues.
336-365: LGTM: Terminal creation logging is thorough.The debug logs around terminal creation and the initialization wait provide good visibility into the async terminal setup flow.
396-403: LGTM: Command execution is well-instrumented.The debug logs before and after the write operation provide clear visibility into command execution timing.
405-424: LGTM: Renderer notification and success flow are correct.The notification flow properly informs the renderer about the auth terminal creation, and the success message provides helpful context.
367-394: Ignoring theenvreturn value creates a fallback risk if Claude CLI detection fails.Line 371 only destructures
commandfromgetClaudeCliInvocationAsync(), discarding theenvthat contains augmented PATH. While the detection logic (detectClaudeAsync) typically returns absolute paths (from Homebrew, NVM, platform standard locations, orfindExecutableAsync), the code's fallback behavior (line 990 in cli-tool-manager.ts) returns just the string'claude'if detection fails. In that scenario, ignoring the env would cause the command to fail.The comment on line 368 assumes the full path will always be available, but defensive handling of the fallback case would be safer. Compare this with the correct pattern used in
claude-integration-handler.ts(lines 542, 625), where bothcommandandenvare destructured and env is passed via pathPrefix.
|
Pushed additional fixes for Auto-Claude PR Review feedback: ✅ Console.log → DebugLog Replacements
✅ Additional Fixes
⏳ RemainingThe extensive i18n for all hardcoded UI strings in |
|
@aslaker Ready for your final look! 👀 I've addressed the feedback from the Auto-Claude review (console.logs -> debugLog, magic numbers, error handling consistency). As noted above, I'm proposing we handle the remaining extensive i18n changes (30+ keys) in a follow-up PR to keep this freeze fix focused. Could you review and approve so we can merge? 🙏 |
|
|
aslaker
left a comment
aslaker
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 5 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 5 previous findings addressed
- ❌ Unresolved: 5 previous findings remain
- 🆕 New Issues: 10 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 1 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- quality: [UNRESOLVED] Race condition: rapid successive calls can interleave chunks
- quality: Hardcoded UI strings violate i18n guidelines
- quality: Hardcoded UI strings violate i18n guidelines
Verdict
CI Status: ✅ All 16 checks passing - CI does not block this PR.
Progress Made:
- 6 of 10 previous findings RESOLVED (chunked write implementation, error handling consistency, magic numbers, login command logging, terminal validity checks)
- Alert dialogs replaced with toast notifications ✓
- Toast messages properly use i18n translation keys ✓
- Most console.log statements converted to debugLog ✓
Blocking Issues (MEDIUM severity):
-
Hardcoded UI strings in SDKRateLimitModal.tsx (finding e26db54eba18, partially resolved) - ~15+ hardcoded English strings remain despite toast messages using i18n. Inconsistent with RateLimitModal.tsx which properly uses translations.
-
Hardcoded UI strings in OAuthStep.tsx (finding 706370be0a87, partially resolved) - ~25+ hardcoded English strings remain. Headers, labels, badges, and button text need translation keys.
-
Race condition confirmed valid (finding 43d2e30dfd5c) - finding-validator confirmed the interleaving issue is real. While severity is situational (edge case for terminal writes), this is a documented design gap.
Non-blocking Issues (LOW severity):
- 8 remaining console.error/console.warn calls should use debugError/debugLog for consistency
Required Actions:
- Add i18n translation keys for UI strings in SDKRateLimitModal.tsx (match RateLimitModal.tsx pattern)
- Add i18n translation keys for UI strings in OAuthStep.tsx
- (Optional) Convert remaining console.error to debugError for consistency
- (Future consideration) Add write serialization to chunked PTY writes if interleaving causes issues
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (11 selected of 11 total)
🟡 [43d2e30dfd5c] [MEDIUM] [UNRESOLVED] Race condition: rapid successive calls can interleave chunks
📁 apps/frontend/src/main/terminal/pty-manager.ts:190
When writeToPty is called multiple times rapidly with data > 1000 bytes, chunks from different calls can interleave because setImmediate callbacks are queued in FIFO order. For example, two calls with 'AAAA...' and 'BBBB...' could result in 'AABBAA...' being written to the PTY.
Resolution note: No mutex, queue, or serialization in writeToPty (lines 167-216). setImmediate schedules chunks asynchronously with no inter-call coordination.
Suggested fix:
Implement a write queue at the terminal level that ensures one large write completes before the next begins. Or use a mutex pattern to serialize chunked writes per terminal.
🔵 [NEW-001] [LOW] console.error instead of debugError
📁 apps/frontend/src/main/ipc-handlers/terminal-handlers.ts:59
Line 59 uses console.error instead of the imported debugError function for consistency.
Suggested fix:
Replace with debugError
🔵 [NEW-002] [LOW] Duplicate logging: console.error and debugError
📁 apps/frontend/src/main/ipc-handlers/terminal-handlers.ts:426
Exception handling uses both console.error (line 426) and debugError (line 427) for the same error.
Suggested fix:
Remove duplicate console.error
🔵 [NEW-003] [LOW] console.error instead of debugError
📁 apps/frontend/src/main/ipc-handlers/terminal-handlers.ts:660
Error handling in TERMINAL_RESUME_CLAUDE uses console.error instead of debugError.
Suggested fix:
Replace with debugError
🔵 [NEW-004] [LOW] console.error instead of debugError
📁 apps/frontend/src/main/ipc-handlers/terminal-handlers.ts:671
Error handling in TERMINAL_ACTIVATE_DEFERRED_RESUME uses console.error instead of debugError.
Suggested fix:
Replace with debugError
🔵 [NEW-005] [LOW] console.error instead of debugError
📁 apps/frontend/src/main/terminal/terminal-manager.ts:88
Uses console.error in catch handler despite importing debugError.
Suggested fix:
Replace with debugError
🔵 [NEW-006] [LOW] console.error instead of debugError
📁 apps/frontend/src/main/terminal/terminal-manager.ts:320
Another console.error usage that should use debugError for consistency.
Suggested fix:
Replace with debugError
🔵 [NEW-007] [LOW] console.warn instead of debugLog
📁 apps/frontend/src/main/terminal/pty-manager.ts:88
Uses console.warn for shell spawning info instead of debugLog.
Suggested fix:
Replace with debugLog
🔵 [NEW-008] [LOW] console.warn instead of debugLog
📁 apps/frontend/src/main/terminal/pty-manager.ts:141
Uses console.warn for terminal exit logging instead of debugLog.
Suggested fix:
Replace with debugLog
🟡 [NEW-009] [MEDIUM] Hardcoded UI strings violate i18n guidelines
📁 apps/frontend/src/renderer/components/SDKRateLimitModal.tsx:247
SDKRateLimitModal has ~15+ hardcoded English strings that should use translation keys. Examples: 'Claude Code Rate Limit', 'Upgrade to Pro for Higher Limits', 'Switch Account & Retry', 'Select account...', 'Close', etc. This is inconsistent with RateLimitModal.tsx which properly uses i18n.
Suggested fix:
Apply same i18n pattern as RateLimitModal.tsx
🟡 [NEW-010] [MEDIUM] Hardcoded UI strings violate i18n guidelines
📁 apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:329
OAuthStep has ~25+ hardcoded English strings despite importing useTranslation. Examples: 'Configure Claude Authentication', 'Add your Claude accounts', 'No accounts configured yet', badge labels ('Default', 'Active', 'Authenticated'), button text, etc. Toast messages correctly use i18n but UI strings do not.
Suggested fix:
Add translation keys for all UI strings in onboarding.json
This review was generated by Auto Claude.
…r + i18n - Replace 8 remaining console.error/warn calls with debugError/debugLog: - terminal-handlers.ts: lines 59, 426, 660, 671 - terminal-manager.ts: lines 88, 320 - pty-manager.ts: lines 88, 141 - Fixed duplicate logging in exception handler - Add comprehensive i18n for SDKRateLimitModal.tsx (~20 strings): - Added rateLimit.sdk.* keys with swap notifications, buttons, labels - EN + FR translations in common.json - Add comprehensive i18n for OAuthStep.tsx (~15 strings): - Added oauth.badges.*, oauth.buttons.*, oauth.labels.* keys - EN + FR translations in onboarding.json All MEDIUM severity findings resolved except race condition (deferred).
PR Review Fixes Complete 🎉Addressed all findings from the Auto Claude PR review: ✅ RESOLVEDconsole.error/warn → debugError/debugLog (8 fixes):
i18n for SDKRateLimitModal.tsx (~20 strings):
i18n for OAuthStep.tsx (~15 strings):
🔄 DEFERRED (Future consideration)
Commit: @aslaker Ready for re-review! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/frontend/src/renderer/components/SDKRateLimitModal.tsx (1)
247-499: Good i18n coverage with remaining hardcoded strings to address.Most user-facing strings now use translation keys (
t()), which is a significant improvement. However, several hardcoded strings remain in this file:
- Line 329-330: "Recommended: ..." and "Switch to another Claude account..."
- Line 339: "Select account..."
- Line 375: "Add new account..."
- Line 424: "Add another Claude subscription..."
- Line 431: "Add another account:" / "Connect a Claude account:"
- Line 435: "Account name (e.g., Work, Personal)"
- Line 461: "This will open Claude login..."
- Lines 469-472, 481: Upgrade-related strings
Per the PR comments, you've explicitly scoped this work and plan to defer the remaining ~30 keys to a follow-up PR. This aligns with the project's practice of deferring pre-existing i18n issues to focused cleanup passes.
Based on learnings, pre-existing i18n issues can be deferred to future cleanup passes when they don't block the current PR's objectives.
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx (1)
101-117: FixuseEffectdeps for the OAuth token listener (likely eslint react-hooks warning / stale closures).This effect uses
loadClaudeProfiles()andt(...)but only depends ontoast(Line 116). Please either memoizeloadClaudeProfilesand include deps, or refactor to avoid capturing unstable references.Proposed fix (memoize loader + correct deps)
-import { useState, useEffect } from 'react'; +import { useState, useEffect, useCallback } from 'react'; @@ - const loadClaudeProfiles = async () => { + const loadClaudeProfiles = useCallback(async () => { setIsLoadingProfiles(true); setError(null); try { const result = await window.electronAPI.getClaudeProfiles(); if (result.success && result.data) { setClaudeProfiles(result.data.profiles); setActiveProfileId(result.data.activeProfileId); // Also update the global store await loadGlobalClaudeProfiles(); } } catch (err) { setError(err instanceof Error ? err.message : 'Failed to load profiles'); } finally { setIsLoadingProfiles(false); } - }; + }, []); @@ - }, [toast]); + }, [loadClaudeProfiles, t, toast]);
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/pty-manager.ts:
- Around line 167-206: writeToPty has a race where concurrent chunked writes
interleave and partial failures continue sending remaining chunks; fix by adding
a per-terminal FIFO write queue (e.g., attach a terminal._writeQueue or
terminal.writeQueue) that enqueues the entire data string as one task and
processes tasks serially so only one chunked writer runs at a time; ensure the
chunked processor checks a real liveness/writable predicate on terminal.pty (not
just truthiness) before each chunk, abort and discard remaining chunks for that
queued task on any write error (log the error) so partial commands are not sent,
then proceed to the next queued task; keep CHUNKED_WRITE_THRESHOLD and
CHUNK_SIZE logic but move chunking to the queue worker and schedule next chunk
via setImmediate to yield.
- Around line 155-162: Update the block comment above CHUNKED_WRITE_THRESHOLD
and CHUNK_SIZE to explicitly state that chunking uses setImmediate between
chunks to intentionally yield to the event loop (to avoid blocking), that
CHUNK_SIZE = 100 is a conservative value chosen for yielding rather than
latency/throughput optimization, and that CHUNKED_WRITE_THRESHOLD = 1000 ensures
small writes bypass chunking; note that performance tuning is optional and not
required for correctness.
In @apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:
- Around line 159-165: The toast is displaying raw error strings
(initResult.error / result.error) which may expose sensitive internals; instead,
log the full error to the console or a logger (e.g., console.error or a provided
logger) inside the same block where loadClaudeProfiles() and toast(...) are
called, and pass a localized, user-safe message to toast using
t('oauth.toast.authStartFailed') or t('oauth.toast.tryAgain'); for the other
occurrences (the blocks around the result.error usages at ~lines 240-245 and
296-301) do the same: log the raw error and show only a sanitized/localized
message in the toast (optionally include a non-sensitive short code if you
extract/sanitize it before showing).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
apps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/shared/i18n/locales/en/common.jsonapps/frontend/src/shared/i18n/locales/en/onboarding.jsonapps/frontend/src/shared/i18n/locales/fr/common.jsonapps/frontend/src/shared/i18n/locales/fr/onboarding.json
🧰 Additional context used
📓 Path-based instructions (4)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/main/ipc-handlers/terminal-handlers.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/main/ipc-handlers/terminal-handlers.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/main/terminal/terminal-manager.tsapps/frontend/src/main/ipc-handlers/terminal-handlers.ts
apps/frontend/src/shared/i18n/locales/**/*.json
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/shared/i18n/locales/**/*.json: Store translation strings in namespace-organized JSON files atapps/frontend/src/shared/i18n/locales/{lang}/*.jsonfor each supported language
When implementing new frontend features, add translation keys to all language files (minimum: en/.json and fr/.json)
Files:
apps/frontend/src/shared/i18n/locales/en/onboarding.jsonapps/frontend/src/shared/i18n/locales/en/common.jsonapps/frontend/src/shared/i18n/locales/fr/onboarding.jsonapps/frontend/src/shared/i18n/locales/fr/common.json
🧠 Learnings (6)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/frontend/src/**/*.{ts,tsx,jsx} : Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Applied to files:
apps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/onboarding/OAuthStep.tsx
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/frontend/src/**/*.{ts,tsx} : Use `useTranslation()` hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Applied to files:
apps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/onboarding/OAuthStep.tsx
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/frontend/src/shared/i18n/locales/**/*.json : When implementing new frontend features, add translation keys to all language files (minimum: en/*.json and fr/*.json)
Applied to files:
apps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/shared/i18n/locales/en/onboarding.jsonapps/frontend/src/shared/i18n/locales/en/common.jsonapps/frontend/src/shared/i18n/locales/fr/onboarding.jsonapps/frontend/src/shared/i18n/locales/fr/common.json
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/onboarding/OAuthStep.tsx
📚 Learning: 2025-12-22T22:43:58.052Z
Learnt from: AndyMik90
Repo: AndyMik90/Auto-Claude PR: 150
File: apps/frontend/src/renderer/components/settings/ProjectSettingsContent.tsx:0-0
Timestamp: 2025-12-22T22:43:58.052Z
Learning: In the AndyMik90/Auto-Claude repository, the ESLint configuration only allows `console.warn` and `console.error` levels, not `console.log` or `console.info`.
Applied to files:
apps/frontend/src/main/terminal/terminal-manager.ts
🧬 Code graph analysis (5)
apps/frontend/src/renderer/components/SDKRateLimitModal.tsx (2)
apps/frontend/src/renderer/hooks/use-toast.ts (2)
useToast(192-192)toast(192-192)apps/frontend/src/renderer/components/ui/dialog.tsx (3)
DialogTitle(124-124)DialogDescription(125-125)DialogFooter(123-123)
apps/frontend/src/main/terminal/pty-manager.ts (2)
apps/frontend/src/main/terminal/types.ts (1)
TerminalProcess(8-22)apps/frontend/src/shared/utils/debug-logger.ts (1)
debugError(25-29)
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx (2)
apps/frontend/src/renderer/hooks/use-toast.ts (2)
useToast(192-192)toast(192-192).design-system/src/lib/icons.ts (2)
Star(34-34)Check(21-21)
apps/frontend/src/main/terminal/terminal-manager.ts (1)
apps/frontend/src/shared/utils/debug-logger.ts (1)
debugError(25-29)
apps/frontend/src/main/ipc-handlers/terminal-handlers.ts (4)
apps/frontend/src/shared/utils/debug-logger.ts (1)
debugError(25-29)apps/frontend/src/main/claude-profile-manager.ts (1)
getClaudeProfileManager(560-565)apps/frontend/src/main/claude-cli-utils.ts (1)
getClaudeCliInvocationAsync(66-77)apps/frontend/src/shared/utils/shell-escape.ts (1)
escapeShellArg(24-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (13)
apps/frontend/src/shared/i18n/locales/fr/common.json (1)
150-183: LGTM! French translations are accurate with complete key parity.The French translations are grammatically correct, follow proper French typographic conventions (guillemets, accents), and all variable placeholders are correctly preserved. Key parity with the English locale is confirmed—all keys in
rateLimit.toastandrateLimit.sdkmatch exactly betweenfr/common.jsonanden/common.json.apps/frontend/src/shared/i18n/locales/en/common.json (1)
150-183: LGTM! Clean i18n structure for rate limit notifications.The new translation keys are well-organized under
rateLimit.sourceswith clear separation betweentoast(notification messages) andsdk(modal UI strings). The structure is consistent with existing patterns, and according to the PR context, corresponding French translations have been added.apps/frontend/src/shared/i18n/locales/fr/onboarding.json (1)
34-77: LGTM! Complete French translations for OAuth flow.The French translations are comprehensive and properly structured, mirroring the English locale structure. This fulfills the coding guideline requirement to add translation keys to all language files (minimum en + fr).
apps/frontend/src/main/terminal/terminal-manager.ts (1)
20-20: LGTM! Consistent use of debug logging utilities.The changes correctly replace
console.errorwithdebugErrorand add helpful debug traces in the write flow. This aligns with the project's ESLint configuration that restricts console usage to warn/error levels only.Based on learnings, the project prefers centralized debug logging utilities over direct console usage.
Also applies to: 88-88, 124-132, 320-320
apps/frontend/src/renderer/components/SDKRateLimitModal.tsx (2)
2-2: LGTM! Proper setup for i18n and non-blocking notifications.The imports and hook initialization correctly establish i18n support and toast notifications, replacing the previous blocking alert dialogs with a better UX.
Also applies to: 25-25, 60-61
167-187: Excellent improvement: non-blocking toasts with i18n.The replacement of blocking
alert()dialogs withtoast()notifications significantly improves UX by:
- Not blocking the event loop during profile authentication
- Using localized messages via
t()calls- Providing consistent visual feedback
The error handling properly uses the
destructivevariant for error toasts.apps/frontend/src/main/ipc-handlers/terminal-handlers.ts (2)
59-59: LGTM! Consistent error logging with debug utilities.The replacement of
console.errorwithdebugErroraligns with project logging standards and provides centralized debug control.Also applies to: 659-659, 670-670
315-427: Excellent diagnostic improvements and simplified CLI invocation.The changes provide comprehensive debugging for the Claude profile initialization flow:
Debug logging enhancements:
- Traces each step of profile initialization (profile lookup, config dir creation, terminal creation, command construction)
- Logs key decision points and data (profile existence, terminal creation success, command details)
- Appropriate detail level for troubleshooting complex OAuth flows without being excessive
CLI invocation simplification (lines 369-398):
- Now uses
getClaudeCliInvocationAsync()to obtain the absolute Claude CLI path- Eliminates PATH manipulation in favor of direct absolute path usage
- Properly escapes commands using OS-specific functions:
escapeShellArgWindows()for Windows cmd.exeescapeShellArg()for POSIX shells (bash/zsh)- Correct environment variable syntax for each OS:
- Windows:
set "VAR=value" && %VAR%- POSIX:
export VAR=value && $VARThe security posture is solid with proper escaping, and the diagnostic improvements will significantly aid troubleshooting.
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx (2)
30-31: Toast integration is clean and localized.Also applies to: 45-47
107-112: Good: replaced blocking alerts / hardcoded UI strings witht('oauth.*')+ non-blocking toasts.Also applies to: 169-174, 250-255, 291-295, 304-308, 328-333, 364-366, 395-396, 457-473, 499-511, 679-694
apps/frontend/src/shared/i18n/locales/en/onboarding.json (1)
34-77: No action needed—EN/FR parity for oauth keys is already complete.Both
en/onboarding.jsonandfr/onboarding.jsoncontain all the sameoauth.*keys with matching structure, includingoauth.toast.authSuccessWithEmailwith the{{email}}placeholder properly preserved in both files.Likely an incorrect or invalid review comment.
apps/frontend/src/main/terminal/pty-manager.ts (2)
13-13: LGTM: Debug logging migration.The replacement of
console.log/console.warnwithdebugLogimproves observability and aligns with the project's enhanced diagnostics strategy.Also applies to: 88-88, 141-141
207-215: Add tests forwriteToPtyerror scenarios and review chunked write logging verbosity.The
writeToPtyfunction (lines 167–216) has no dedicated test coverage. No tests verify error handling for PTY write failures or invalid terminals during chunked writes. Additionally, the chunked write path (triggered for data > 1000 bytes) logs extensively: for a 1.1 KB write, approximately 27 debug messages are emitted (per-chunk logs at lines 190, 193, plus lifecycle logs at 168, 172, 184, 204, 206). Clarify whether this verbosity level is intentional or if it should be reduced to critical lifecycle events only.
| /** | ||
| * Constants for chunked write behavior | ||
| * CHUNKED_WRITE_THRESHOLD: Data larger than this (bytes) will be written in chunks | ||
| * CHUNK_SIZE: Size of each chunk - smaller chunks yield to event loop more frequently | ||
| */ | ||
| const CHUNKED_WRITE_THRESHOLD = 1000; | ||
| const CHUNK_SIZE = 100; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for CHUNK_SIZE and chunked write related references
echo "=== Searching for CHUNK_SIZE and chunked write references ==="
rg -n "CHUNK_SIZE|chunked.*write|CHUNKED_WRITE" --type=ts --type=tsx --type=md -C 2Repository: AndyMik90/Auto-Claude
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Check for performance tests or benchmarks
echo "=== Looking for performance tests or benchmarks ==="
fd -e test.ts -e test.tsx -e spec.ts -e bench.ts | xargs rg -l "chunk|pty|terminal|write" 2>/dev/null || echo "No test files found with those patterns"Repository: AndyMik90/Auto-Claude
Length of output: 3092
🏁 Script executed:
#!/bin/bash
# Get the full context of pty-manager.ts around the constants and their usage
echo "=== Examining pty-manager.ts file size ==="
wc -l apps/frontend/src/main/terminal/pty-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Read the pty-manager.ts file to understand context and usage
echo "=== Reading pty-manager.ts ==="
cat -n apps/frontend/src/main/terminal/pty-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 9408
🏁 Script executed:
#!/bin/bash
# Simpler search for CHUNK_SIZE and related constants
echo "=== Searching for CHUNK_SIZE usage ==="
rg -n "CHUNK_SIZE|CHUNKED_WRITE_THRESHOLD|setImmediate" apps/frontend/src/main/terminal/ -C 3Repository: AndyMik90/Auto-Claude
Length of output: 4038
The chunked write approach is sound, though the rationale for specific chunk size could be clearer.
The constants are well-extracted and documented. However, the logic is intentionally designed to yield to the event loop via setImmediate between chunks—this is not a latency concern but the intended behavior to prevent blocking. CHUNK_SIZE of 100 bytes is conservative by design for this purpose, and the 1000-byte threshold ensures small writes bypass chunking entirely. While performance testing could validate whether the chunk size is optimal for actual throughput, it is not necessary for correctness, as the current values are reasonable for a terminal I/O operation.
🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/pty-manager.ts around lines 155 - 162,
Update the block comment above CHUNKED_WRITE_THRESHOLD and CHUNK_SIZE to
explicitly state that chunking uses setImmediate between chunks to intentionally
yield to the event loop (to avoid blocking), that CHUNK_SIZE = 100 is a
conservative value chosen for yielding rather than latency/throughput
optimization, and that CHUNKED_WRITE_THRESHOLD = 1000 ensures small writes
bypass chunking; note that performance tuning is optional and not required for
correctness.
| await loadClaudeProfiles(); | ||
| alert(`Failed to start authentication: ${initResult.error || 'Please try again.'}`); | ||
| toast({ | ||
| variant: 'destructive', | ||
| title: t('oauth.toast.authStartFailed'), | ||
| description: initResult.error || t('oauth.toast.tryAgain'), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Avoid showing raw *.error strings in user-facing toasts unless they’re guaranteed “safe”.
initResult.error / result.error may contain technical details (paths, internal codes, etc.). Prefer logging the raw error and showing a localized, user-safe message in the toast.
Safer UX pattern (keep details out of the toast)
toast({
variant: 'destructive',
title: t('oauth.toast.authStartFailed'),
- description: initResult.error || t('oauth.toast.tryAgain'),
+ description: t('oauth.toast.tryAgain'),
});Also applies to: 240-245, 296-301
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx around lines
159 - 165, The toast is displaying raw error strings (initResult.error /
result.error) which may expose sensitive internals; instead, log the full error
to the console or a logger (e.g., console.error or a provided logger) inside the
same block where loadClaudeProfiles() and toast(...) are called, and pass a
localized, user-safe message to toast using t('oauth.toast.authStartFailed') or
t('oauth.toast.tryAgain'); for the other occurrences (the blocks around the
result.error usages at ~lines 240-245 and 296-301) do the same: log the raw
error and show only a sanitized/localized message in the toast (optionally
include a non-sensitive short code if you extract/sanitize it before showing).
AlexMadera
left a comment
AlexMadera
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 2 issue(s) require attention.
2 issue(s) must be addressed (0 required, 2 recommended), 3 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 2 issue(s)
- Low: 3 issue(s)
Generated by Auto Claude PR Review
Findings (5 selected of 5 total)
🟡 [4051d875f312] [MEDIUM] Theoretical race condition in chunked write mechanism
📁 apps/frontend/src/main/terminal/pty-manager.ts:176
The chunked write implementation uses closures over offset and data variables. If writeToPty() is called concurrently for the same terminal before the first write completes, chunks from different calls could interleave, causing data corruption. The function returns immediately while chunks continue writing asynchronously.
Suggested fix:
Add a write queue per terminal or a mutex lock to serialize concurrent writes. Alternatively, document that writeToPty() must not be called concurrently and add an assertion to detect violations.
🟡 [b56431a47a41] [MEDIUM] Excessive debug logging in writeToPty function
📁 apps/frontend/src/main/terminal/pty-manager.ts:167
The writeToPty function contains 13 debugLog/debugError calls. This is excessive for production code and adds noise. The detailed logging of every chunk (including offset and size) will flood logs during normal operation.
Suggested fix:
Reduce logging to 2-3 key points: function entry, chunked write start, and completion/error. Remove per-chunk logging. Consider gating debug logs behind a DEBUG environment variable.
🔵 [624ecfb26ca1] [LOW] Magic numbers for chunk size configuration
📁 apps/frontend/src/main/terminal/pty-manager.ts:160
CHUNKED_WRITE_THRESHOLD (1000) and CHUNK_SIZE (100) are magic numbers defined as constants but without documentation explaining their rationale. These values affect performance and the effectiveness of preventing UI freezes.
Suggested fix:
Add JSDoc comments explaining the trade-offs: larger CHUNK_SIZE reduces overhead but may still cause brief blocking; smaller chunks yield more often but increase write overhead. Mention that these were empirically chosen.
🔵 [661c197b066d] [LOW] Excessive debug logging in IPC handler
📁 apps/frontend/src/main/ipc-handlers/terminal-handlers.ts:315
The CLAUDE_PROFILE_INITIALIZE handler contains 20+ debugLog calls. This is overly verbose for production code.
Suggested fix:
Reduce to essential logging points: handler entry, profile lookup success/failure, terminal creation success/failure, and command execution. Remove step-by-step logging.
🔵 [e8001b0b22a5] [LOW] Inconsistent error logging - console.error not converted to debugLog
📁 apps/frontend/src/renderer/components/settings/IntegrationSettings.tsx:232
While most logging was converted to debugLog, line 232 still uses console.error. This is inconsistent with the project's logging utility.
Suggested fix:
Replace with debugError('[IntegrationSettings] Failed to authenticate profile:', err) for consistency.
This review was generated by Auto Claude.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 0 previous findings addressed
- ❌ Unresolved: 1 previous findings remain
- 🆕 New Issues: 1 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 1 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: Race condition in chunked PTY writes
Verdict
CI Status: ⏳ 1 CI check pending - Cannot give READY_TO_MERGE until CI completes.
Blocking Issues:
- NEW-002 (LOW, CONFIRMED_VALID): Missing 't' in useEffect dependency array in OAuthStep.tsx:116. Was flagged in previous review and remains unfixed. Should be
[t, toast]not[toast]. - NEW-001 (MEDIUM): Race condition in chunked PTY write implementation. Concurrent large writes can interleave chunks, corrupting terminal input.
Progress Acknowledged:
- ✅ console.error/warn → debugError/debugLog (8 fixes)
- ✅ Alert dialogs replaced with i18n toast notifications
- ✅ Translation keys added to en/fr locale files
Required Actions:
- Wait for CI to complete
- Fix OAuthStep.tsx line 116: change
[toast]to[t, toast] - Address race condition in pty-manager.ts chunked writes (add write serialization per terminal)
Out-of-Scope Issues Filtered:
- CodeRabbit's i18n findings for lines 328, 339, 375, etc. in SDKRateLimitModal.tsx are in unchanged code sections (pre-existing)
- Hardcoded strings in Terminal.tsx, settings-store.ts, TaskDetailModal.tsx are in files not modified by this PR
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (2 selected of 2 total)
🔵 [NEW-002] [LOW] [UNRESOLVED] Previous finding NEW-001 not addressed: Missing 't' in useEffect dependency array
📁 apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:116
The useEffect at line 102-116 uses the translation function 't' inside the callback but only includes 'toast' in the dependency array. IntegrationSettings.tsx correctly uses [t, toast] for the same pattern at line 98. This is a minor React hooks exhaustive-deps violation, but the 't' function from useTranslation is stable and unlikely to cause runtime issues.
Resolution note: // OAuthStep.tsx lines 102-116:
useEffect(() => {
const unsubscribe = window.electronAPI.onTerminalOAuthToken(async (info) => {
if (info.success && info.profileId) {
await loadClaudeProfiles();
toast({
title: t('oauth.toast.authSuccess'),
description: info.email ? t('oauth.toast.authSuccessWithEmail', { email: info.email }) : t('oauth.toast.authSuccessGeneric'),
});
}
});
return unsubscribe;
}, [toast]); // <-- Still missing 't' in dependency array
Suggested fix:
Add 't' to the dependency array: }, [toast, t]); (matching IntegrationSettings.tsx pattern)
🟡 [NEW-001] [MEDIUM] Race condition in chunked PTY writes
📁 apps/frontend/src/main/terminal/pty-manager.ts:167
The chunked write implementation lacks synchronization. When writeToPty is called with large data (>1000 bytes), it schedules chunks via setImmediate and returns immediately. If another write call occurs before all chunks are written, the data from both writes will interleave at the PTY level. This is possible because TERMINAL_INPUT uses ipcMain.on which can fire multiple events asynchronously. While unlikely in normal typing, rapid paste operations combined with user input could trigger this.
Suggested fix:
Add a per-terminal write queue that ensures writes complete in order. Alternatively, use a Map<terminalId, Promise> to track pending writes and chain new writes to wait for completion.
This review was generated by Auto Claude.
…e.error - Fix race condition in chunked PTY writes by serializing writes per terminal using Promise chaining (prevents interleaving of concurrent large writes) - Fix missing 't' in useEffect dependency array in OAuthStep.tsx - Convert all remaining console.error calls to debugError for consistency: - IntegrationSettings.tsx (9 occurrences) - RateLimitModal.tsx (3 occurrences) - SDKRateLimitModal.tsx (4 occurrences) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: Black Circle Sentinel <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/components/RateLimitModal.tsx (1)
271-281: Fix broken “focus add account input” logic after i18n placeholder change.
Line 340 localizes the placeholder, but Lines 274-275 still doquerySelector('input[placeholder*="Account name"]'), which will stop working for non-English locales. Use aref, stableid, ordata-*attribute instead.Proposed diff (ref-based)
-import { useEffect, useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; ... export function RateLimitModal() { const { t } = useTranslation('common'); + const addAccountInputRef = useRef<HTMLInputElement | null>(null); ... <DropdownMenuItem onClick={() => { - // Focus the add account input - const input = document.querySelector('input[placeholder*="Account name"]') as HTMLInputElement; - if (input) input.focus(); + addAccountInputRef.current?.focus(); }} ... <Input + ref={addAccountInputRef} placeholder={t('rateLimit.accountNamePlaceholder')} value={newProfileName}Also applies to: 339-349
🤖 Fix all issues with AI agents
In @apps/frontend/src/main/terminal/pty-manager.ts:
- Around line 155-246: The threshold comment and logic mismatch:
CHUNKED_WRITE_THRESHOLD currently compares UTF-16 code units (data.length) not
bytes — update the comment to say “UTF-16 code units” or change the check to use
Buffer.byteLength(data, 'utf8') and, if chunking by bytes, adjust chunk slicing
accordingly; inside performWrite replace offset += CHUNK_SIZE with offset +=
chunk.length to handle the final partial chunk correctly; harden the write queue
in writeToPty by preventing a poisoned chain: derive previousWrite with const
previousWrite = (pendingWrites.get(terminal.id) || Promise.resolve()).catch(()
=> {}) so any rejection is swallowed before .then(() => performWrite(...)); keep
references: CHUNKED_WRITE_THRESHOLD, CHUNK_SIZE, performWrite, writeToPty, and
pendingWrites.
In @apps/frontend/src/renderer/components/SDKRateLimitModal.tsx:
- Around line 279-285: Replace hardcoded English fallbacks in SDKRateLimitModal:
remove uses of currentProfile?.name || 'your account' and currentProfile?.name
|| 'Default' and instead pass a localized fallback via the t(...) function
(e.g., use t('...') keys for "your account" and "Default") when constructing the
translated strings; update the translation keys used (or add new keys) so the
fallback text is localized, and ensure the t(...) call is used where
currentProfile?.name may be undefined (affecting the JSX around
t('rateLimit.sdk.operationStopped') and the other occurrence).
In @apps/frontend/src/renderer/components/settings/IntegrationSettings.tsx:
- Around line 215-241: The debug log in handleAuthenticateProfile currently
prints the entire initResult from window.electronAPI.initializeClaudeProfile,
which can leak sensitive tokens/PII; change the debugLog call that references
"IPC returned" to only log a sanitized subset (e.g., initResult.success and
initResult.error or a small DTO like { success: initResult.success, error:
initResult.error }) and avoid including any raw payload or token fields so only
non-sensitive status/error info is emitted.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
apps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/renderer/components/settings/IntegrationSettings.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/settings/IntegrationSettings.tsxapps/frontend/src/main/terminal/pty-manager.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/settings/IntegrationSettings.tsxapps/frontend/src/main/terminal/pty-manager.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/settings/IntegrationSettings.tsxapps/frontend/src/main/terminal/pty-manager.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/frontend/src/**/*.{ts,tsx,jsx} : Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Applied to files:
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/SDKRateLimitModal.tsx
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/frontend/src/shared/i18n/locales/**/*.json : When implementing new frontend features, add translation keys to all language files (minimum: en/*.json and fr/*.json)
Applied to files:
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/SDKRateLimitModal.tsx
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/frontend/src/**/*.{ts,tsx} : Use `useTranslation()` hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Applied to files:
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/SDKRateLimitModal.tsx
📚 Learning: 2026-01-04T23:59:45.209Z
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:45.209Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings) can be deferred for future i18n cleanup passes. Do not fix such issues in PRs that do not introduce new i18n violations, especially in frontend TSX components (e.g., apps/frontend/**/*.tsx). If a PR adds new i18n violations, address them in that PR.
Applied to files:
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsxapps/frontend/src/renderer/components/RateLimitModal.tsxapps/frontend/src/renderer/components/SDKRateLimitModal.tsxapps/frontend/src/renderer/components/settings/IntegrationSettings.tsx
📚 Learning: 2026-01-04T12:37:52.222Z
Learnt from: StillKnotKnown
Repo: AndyMik90/Auto-Claude PR: 648
File: apps/frontend/src/renderer/components/settings/ModelSearchableSelect.tsx:262-265
Timestamp: 2026-01-04T12:37:52.222Z
Learning: Dropdown components in apps/frontend/src/renderer/components/settings use a fixed max-height (e.g., max-h-60) with internal overflow-y-auto scrolling. This pattern is intentional and prevents dropdowns from extending beyond the viewport; do not add dynamic viewport positioning or clipping-prevention logic for these components. If you add new dropdowns in this area, follow the same fixed-height + internal-scroll approach for consistency.
Applied to files:
apps/frontend/src/renderer/components/settings/IntegrationSettings.tsx
🧬 Code graph analysis (3)
apps/frontend/src/renderer/components/RateLimitModal.tsx (2)
apps/frontend/src/renderer/hooks/use-toast.ts (2)
useToast(192-192)toast(192-192)apps/frontend/src/shared/utils/debug-logger.ts (1)
debugError(25-29)
apps/frontend/src/renderer/components/SDKRateLimitModal.tsx (3)
apps/frontend/src/renderer/hooks/use-toast.ts (2)
useToast(192-192)toast(192-192)apps/frontend/src/shared/utils/debug-logger.ts (1)
debugError(25-29)apps/frontend/src/renderer/components/ui/dialog.tsx (3)
DialogTitle(124-124)DialogDescription(125-125)DialogFooter(123-123)
apps/frontend/src/main/terminal/pty-manager.ts (2)
apps/frontend/src/main/terminal/types.ts (1)
TerminalProcess(8-22)apps/frontend/src/shared/utils/debug-logger.ts (1)
debugError(25-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (7)
apps/frontend/src/main/terminal/pty-manager.ts (1)
13-13: Debug logging migration looks good; keep logs non-sensitive.
The newdebugLogcalls are helpful for diagnosing shell selection and PTY exits. Just ensure no secrets (tokens/paths that might be sensitive) get added to these logs over time.Also applies to: 88-88, 141-141
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx (3)
30-31: OAuth completion toast +useEffectdeps fix looks correct.
Addingtoastto the deps (Line 116) avoids stale closures, and the success toast messaging is appropriately non-blocking.Also applies to: 46-47, 102-117
160-174: Consistent non-blocking error handling via destructive toasts—LGTM.
The failure paths now surface user feedback without freezing the UI, and still preserveresult.errorwhen available.Also applies to: 240-255, 291-309
329-333: i18n key usage for updated UI text is aligned with frontend guidelines.
The moved strings tot('oauth.*')follow the expected namespacing pattern.Also applies to: 365-366, 395-396, 457-473, 499-511, 679-694
apps/frontend/src/renderer/components/SDKRateLimitModal.tsx (1)
2-3: Toast +debugErrorintegration is consistent with the new UX direction.
Non-blocking notifications and conditional logging match the other updated modals.Also applies to: 25-27, 61-63, 109-133, 168-188, 216-217
apps/frontend/src/renderer/components/RateLimitModal.tsx (1)
25-27: Toast UX +debugErrorlogging changes are solid.
This removes blocking dialogs on the critical rate-limit path and preserves error visibility in debug mode.Also applies to: 34-35, 63-87, 122-142
apps/frontend/src/renderer/components/settings/IntegrationSettings.tsx (1)
32-34: Toast-based feedback +debugErrormigration is consistent and avoids UI-blocking flows.
The success/failure paths now surface actionable feedback withoutalert()interruptions.Also applies to: 48-49, 83-99, 144-158, 223-241, 274-292, 320-333
| /** | ||
| * Constants for chunked write behavior | ||
| * CHUNKED_WRITE_THRESHOLD: Data larger than this (bytes) will be written in chunks | ||
| * CHUNK_SIZE: Size of each chunk - smaller chunks yield to event loop more frequently | ||
| */ | ||
| const CHUNKED_WRITE_THRESHOLD = 1000; | ||
| const CHUNK_SIZE = 100; | ||
|
|
||
| /** | ||
| * Write queue per terminal to prevent interleaving of concurrent writes. | ||
| * Maps terminal ID to the last write Promise in the queue. | ||
| */ | ||
| const pendingWrites = new Map<string, Promise<void>>(); | ||
|
|
||
| /** | ||
| * Internal function to perform the actual write (chunked or direct) | ||
| * Returns a Promise that resolves when the write is complete | ||
| */ | ||
| function performWrite(terminal: TerminalProcess, data: string): Promise<void> { | ||
| return new Promise((resolve) => { | ||
| // For large commands, write in chunks to prevent blocking | ||
| if (data.length > CHUNKED_WRITE_THRESHOLD) { | ||
| debugLog('[PtyManager:writeToPty] Large write detected, using chunked write'); | ||
| let offset = 0; | ||
| let chunkNum = 0; | ||
|
|
||
| const writeChunk = () => { | ||
| // Check if terminal is still valid before writing | ||
| if (!terminal.pty) { | ||
| debugError('[PtyManager:writeToPty] Terminal PTY no longer valid, aborting chunked write'); | ||
| resolve(); | ||
| return; | ||
| } | ||
|
|
||
| if (offset >= data.length) { | ||
| debugLog('[PtyManager:writeToPty] Chunked write completed, total chunks:', chunkNum); | ||
| resolve(); | ||
| return; | ||
| } | ||
|
|
||
| const chunk = data.slice(offset, offset + CHUNK_SIZE); | ||
| chunkNum++; | ||
| try { | ||
| terminal.pty.write(chunk); | ||
| offset += CHUNK_SIZE; | ||
| // Use setImmediate to yield to the event loop between chunks | ||
| setImmediate(writeChunk); | ||
| } catch (error) { | ||
| debugError('[PtyManager:writeToPty] Chunked write FAILED at chunk', chunkNum, ':', error); | ||
| resolve(); // Resolve anyway - fire-and-forget semantics | ||
| } | ||
| }; | ||
|
|
||
| // Start the chunked write after yielding | ||
| setImmediate(writeChunk); | ||
| } else { | ||
| try { | ||
| terminal.pty.write(data); | ||
| debugLog('[PtyManager:writeToPty] Write completed successfully'); | ||
| } catch (error) { | ||
| debugError('[PtyManager:writeToPty] Write FAILED:', error); | ||
| } | ||
| resolve(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Write data to a PTY process | ||
| * Uses setImmediate to prevent blocking the event loop on large writes. | ||
| * Serializes writes per terminal to prevent interleaving of concurrent writes. | ||
| */ | ||
| export function writeToPty(terminal: TerminalProcess, data: string): void { | ||
| terminal.pty.write(data); | ||
| debugLog('[PtyManager:writeToPty] About to write to pty, data length:', data.length); | ||
|
|
||
| // Get the previous write Promise for this terminal (if any) | ||
| const previousWrite = pendingWrites.get(terminal.id) || Promise.resolve(); | ||
|
|
||
| // Chain this write after the previous one completes | ||
| const currentWrite = previousWrite.then(() => performWrite(terminal, data)); | ||
|
|
||
| // Update the pending write for this terminal | ||
| pendingWrites.set(terminal.id, currentWrite); | ||
|
|
||
| // Clean up the Map entry when done to prevent memory leaks | ||
| currentWrite.finally(() => { | ||
| // Only clean up if this is still the latest write | ||
| if (pendingWrites.get(terminal.id) === currentWrite) { | ||
| pendingWrites.delete(terminal.id); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix “bytes” vs string.length mismatch; harden the write queue against unexpected rejection.
- Line 157 says “bytes” but the implementation uses
data.length(UTF-16 code units). Either adjust the comment or useBuffer.byteLength(data, 'utf8')for thresholding (and reconsider chunking-by-bytes if you go that route). - Consider making the queue resilient if anything ever rejects (future refactors):
previousWrite.catch(() => {}).then(...), otherwise one rejection can “poison” the chain for that terminal. - Minor: prefer
offset += chunk.lengthinstead ofoffset += CHUNK_SIZEfor clarity/correctness at the tail chunk.
Proposed diff
- * CHUNKED_WRITE_THRESHOLD: Data larger than this (bytes) will be written in chunks
+ * CHUNKED_WRITE_THRESHOLD: Data larger than this (string length) will be written in chunks
* CHUNK_SIZE: Size of each chunk - smaller chunks yield to event loop more frequently export function writeToPty(terminal: TerminalProcess, data: string): void {
debugLog('[PtyManager:writeToPty] About to write to pty, data length:', data.length);
// Get the previous write Promise for this terminal (if any)
const previousWrite = pendingWrites.get(terminal.id) || Promise.resolve();
// Chain this write after the previous one completes
- const currentWrite = previousWrite.then(() => performWrite(terminal, data));
+ const currentWrite = previousWrite
+ .catch(() => undefined)
+ .then(() => performWrite(terminal, data));- const chunk = data.slice(offset, offset + CHUNK_SIZE);
+ const chunk = data.slice(offset, offset + CHUNK_SIZE);
chunkNum++;
try {
terminal.pty.write(chunk);
- offset += CHUNK_SIZE;
+ offset += chunk.length;
// Use setImmediate to yield to the event loop between chunks
setImmediate(writeChunk);🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/pty-manager.ts around lines 155 - 246, The
threshold comment and logic mismatch: CHUNKED_WRITE_THRESHOLD currently compares
UTF-16 code units (data.length) not bytes — update the comment to say “UTF-16
code units” or change the check to use Buffer.byteLength(data, 'utf8') and, if
chunking by bytes, adjust chunk slicing accordingly; inside performWrite replace
offset += CHUNK_SIZE with offset += chunk.length to handle the final partial
chunk correctly; harden the write queue in writeToPty by preventing a poisoned
chain: derive previousWrite with const previousWrite =
(pendingWrites.get(terminal.id) || Promise.resolve()).catch(() => {}) so any
rejection is swallowed before .then(() => performWrite(...)); keep references:
CHUNKED_WRITE_THRESHOLD, CHUNK_SIZE, performWrite, writeToPty, and
pendingWrites.
| <p className="font-medium mb-1">{t('rateLimit.sdk.rateLimitReached')}</p> | ||
| <p> | ||
| The operation was stopped because {currentProfile?.name || 'your account'} reached its usage limit. | ||
| {t('rateLimit.sdk.operationStopped', { account: currentProfile?.name || 'your account' })} | ||
| {hasMultipleProfiles | ||
| ? ' Switch to another account below to continue.' | ||
| : ' Add another Claude account to continue working.'} | ||
| ? ' ' + t('rateLimit.sdk.switchBelow') | ||
| : ' ' + t('rateLimit.sdk.addAccountToContinue')} | ||
| </p> |
There was a problem hiding this comment.
Remove hardcoded English fallbacks inside translated strings.
These are user-facing and will show up in non-English locales:
- Line 281:
currentProfile?.name || 'your account' - Line 490:
currentProfile?.name || 'Default'
Prefer passing a localized fallback (anothert(...)) or handle it inside the translation file.
Proposed diff
- {t('rateLimit.sdk.operationStopped', { account: currentProfile?.name || 'your account' })}
+ {t('rateLimit.sdk.operationStopped', { account: currentProfile?.name || t('rateLimit.sdk.fallbackAccount') })}- {t('rateLimit.sdk.whatHappenedDesc', { source: sourceName.toLowerCase(), account: currentProfile?.name || 'Default' })}
+ {t('rateLimit.sdk.whatHappenedDesc', { source: sourceName.toLowerCase(), account: currentProfile?.name || t('rateLimit.sdk.fallbackDefaultAccount') })}Also applies to: 488-494
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/SDKRateLimitModal.tsx around lines 279
- 285, Replace hardcoded English fallbacks in SDKRateLimitModal: remove uses of
currentProfile?.name || 'your account' and currentProfile?.name || 'Default' and
instead pass a localized fallback via the t(...) function (e.g., use t('...')
keys for "your account" and "Default") when constructing the translated strings;
update the translation keys used (or add new keys) so the fallback text is
localized, and ensure the t(...) call is used where currentProfile?.name may be
undefined (affecting the JSX around t('rateLimit.sdk.operationStopped') and the
other occurrence).
| const handleAuthenticateProfile = async (profileId: string) => { | ||
| debugLog('[IntegrationSettings] handleAuthenticateProfile called for:', profileId); | ||
| setAuthenticatingProfileId(profileId); | ||
| try { | ||
| debugLog('[IntegrationSettings] Calling initializeClaudeProfile IPC...'); | ||
| const initResult = await window.electronAPI.initializeClaudeProfile(profileId); | ||
| debugLog('[IntegrationSettings] IPC returned:', initResult); | ||
| if (!initResult.success) { | ||
| alert(`Failed to start authentication: ${initResult.error || 'Please try again.'}`); | ||
| toast({ | ||
| variant: 'destructive', | ||
| title: t('integrations.toast.authStartFailed'), | ||
| description: initResult.error || t('integrations.toast.tryAgain'), | ||
| }); | ||
| } | ||
| // Note: If successful, the terminal is now visible in the UI via the onTerminalAuthCreated event | ||
| // Users can see the 'claude setup-token' output and complete OAuth flow directly | ||
| } catch (err) { | ||
| console.error('Failed to authenticate profile:', err); | ||
| alert('Failed to start authentication. Please try again.'); | ||
| debugError('[IntegrationSettings] Failed to authenticate profile:', err); | ||
| toast({ | ||
| variant: 'destructive', | ||
| title: t('integrations.toast.authStartFailed'), | ||
| description: t('integrations.toast.tryAgain'), | ||
| }); | ||
| } finally { | ||
| debugLog('[IntegrationSettings] finally block - clearing authenticatingProfileId'); | ||
| setAuthenticatingProfileId(null); | ||
| } |
There was a problem hiding this comment.
Avoid logging full IPC results to prevent accidental token/PII leakage in debug mode.
Line 221 logs the full initResult. Even if “debug-only”, this can still end up in shared logs/screenshots. Prefer logging only { success, error } (or a sanitized subset).
Proposed diff
- debugLog('[IntegrationSettings] IPC returned:', initResult);
+ debugLog('[IntegrationSettings] IPC returned:', {
+ success: initResult?.success,
+ error: initResult?.success ? undefined : initResult?.error,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleAuthenticateProfile = async (profileId: string) => { | |
| debugLog('[IntegrationSettings] handleAuthenticateProfile called for:', profileId); | |
| setAuthenticatingProfileId(profileId); | |
| try { | |
| debugLog('[IntegrationSettings] Calling initializeClaudeProfile IPC...'); | |
| const initResult = await window.electronAPI.initializeClaudeProfile(profileId); | |
| debugLog('[IntegrationSettings] IPC returned:', initResult); | |
| if (!initResult.success) { | |
| alert(`Failed to start authentication: ${initResult.error || 'Please try again.'}`); | |
| toast({ | |
| variant: 'destructive', | |
| title: t('integrations.toast.authStartFailed'), | |
| description: initResult.error || t('integrations.toast.tryAgain'), | |
| }); | |
| } | |
| // Note: If successful, the terminal is now visible in the UI via the onTerminalAuthCreated event | |
| // Users can see the 'claude setup-token' output and complete OAuth flow directly | |
| } catch (err) { | |
| console.error('Failed to authenticate profile:', err); | |
| alert('Failed to start authentication. Please try again.'); | |
| debugError('[IntegrationSettings] Failed to authenticate profile:', err); | |
| toast({ | |
| variant: 'destructive', | |
| title: t('integrations.toast.authStartFailed'), | |
| description: t('integrations.toast.tryAgain'), | |
| }); | |
| } finally { | |
| debugLog('[IntegrationSettings] finally block - clearing authenticatingProfileId'); | |
| setAuthenticatingProfileId(null); | |
| } | |
| const handleAuthenticateProfile = async (profileId: string) => { | |
| debugLog('[IntegrationSettings] handleAuthenticateProfile called for:', profileId); | |
| setAuthenticatingProfileId(profileId); | |
| try { | |
| debugLog('[IntegrationSettings] Calling initializeClaudeProfile IPC...'); | |
| const initResult = await window.electronAPI.initializeClaudeProfile(profileId); | |
| debugLog('[IntegrationSettings] IPC returned:', { | |
| success: initResult?.success, | |
| error: initResult?.success ? undefined : initResult?.error, | |
| }); | |
| if (!initResult.success) { | |
| toast({ | |
| variant: 'destructive', | |
| title: t('integrations.toast.authStartFailed'), | |
| description: initResult.error || t('integrations.toast.tryAgain'), | |
| }); | |
| } | |
| // Note: If successful, the terminal is now visible in the UI via the onTerminalAuthCreated event | |
| // Users can see the 'claude setup-token' output and complete OAuth flow directly | |
| } catch (err) { | |
| debugError('[IntegrationSettings] Failed to authenticate profile:', err); | |
| toast({ | |
| variant: 'destructive', | |
| title: t('integrations.toast.authStartFailed'), | |
| description: t('integrations.toast.tryAgain'), | |
| }); | |
| } finally { | |
| debugLog('[IntegrationSettings] finally block - clearing authenticatingProfileId'); | |
| setAuthenticatingProfileId(null); | |
| } |
🤖 Prompt for AI Agents
In @apps/frontend/src/renderer/components/settings/IntegrationSettings.tsx
around lines 215 - 241, The debug log in handleAuthenticateProfile currently
prints the entire initResult from window.electronAPI.initializeClaudeProfile,
which can leak sensitive tokens/PII; change the debugLog call that references
"IPC returned" to only log a sanitized subset (e.g., initResult.success and
initResult.error or a small DTO like { success: initResult.success, error:
initResult.error }) and avoid including any raw payload or token fields so only
non-sensitive status/error info is emitted.
|
@aslaker All review findings addressed in this revision: Fixed ✅
Ready for re-review when you have a moment! 🙏 |
|
@MikeeBuilds here are two more things that you could fix, but aren't totally necessary and could just be captured as tech debt for later
Otherwise LGTM! 🚀 |
|
I have read the CLA Document and I hereby sign the CLA |
… verification (#3) * Fix/2.7.2 hotfixes (#577) * feat(terminal): respect preferred terminal setting for Windows PTY shell Adds Windows shell selection in the embedded PTY terminal based on the user's preferredTerminal setting from onboarding/settings. On Windows, the terminal preference (PowerShell, Windows Terminal, CMD) now maps to the appropriate shell executable when spawning PTY processes. This ensures the embedded terminal matches user expectations when they select their preferred terminal during setup. - Adds WINDOWS_SHELL_PATHS mapping for powershell, windowsterminal, cmd - Implements getWindowsShell() to find first available shell executable - Falls back to COMSPEC/cmd.exe for 'system' or unknown terminals - Reads preferredTerminal from user settings on each spawn * fix(ci): cache pip wheels to speed up Intel Mac builds The real_ladybug package has no pre-built wheel for macOS x86_64 (Intel), requiring Rust compilation from source on every build. This caused builds to take 5-10+ minutes. Changes: - Remove --no-cache-dir from pip install so wheels get cached - Add pip wheel cache to GitHub Actions cache for all platforms - Include requirements.txt hash in cache keys for proper invalidation - Fix restore-keys to avoid falling back to incompatible old caches After this fix, subsequent Intel Mac builds will use the cached compiled wheel instead of rebuilding from source each time. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * # 🔥 hotfix(electron): restore app functionality on Windows broken by GPU cache errors (#569) ## 📋 Critical Issue | Severity | Impact | Affected Users | |----------|--------|----------------| | 🔴 **CRITICAL** | 🚫 **Non-functional** | 🪟 **Windows users** | On Windows systems, the Electron app failed to create GPU shader and program caches due to filesystem permission errors (**Error 0x5: Access Denied**). This prevented users from initiating the autonomous coding phase, rendering the application **non-functional** for its primary purpose. --- ## 🔍 Root Cause Analysis ### The Problem Chromium's GPU process attempts to create persistent shader caches in the following locations: %LOCALAPPDATA%\auto-claude-ui\GPUCache\ %LOCALAPPDATA%\auto-claude-ui\ShaderCache\ ### Why It Fails | Factor | Description | |--------|-------------| | 🦠 **Antivirus** | Real-time scanning blocks cache directory creation | | 🛡️ **Windows Defender** | Protection policies deny write access | | ☁️ **Sync Software** | OneDrive/Dropbox interferes with AppData folders | | 🔐 **Permissions** | Insufficient rights in default Electron cache paths | ### Error Console Output ❌ ERROR:net\disk_cache\cache_util_win.cc:25] Unable to move the cache: Zugriff verweigert (0x5) ❌ ERROR:gpu\ipc\host\gpu_disk_cache.cc:724] Gpu Cache Creation failed: -2 ❌ ERROR:net\disk_cache\disk_cache.cc:236] Unable to create cache --- ## ✅ Solution Implemented ### 1️⃣ GPU Shader Disk Cache Disabled app.commandLine.appendSwitch('disable-gpu-shader-disk-cache'); - ⚡ Prevents Chromium from writing shader caches to disk - ✅ GPU acceleration remains fully functional - 🎯 Zero performance impact on typical usage ### 2️⃣ GPU Program Disk Cache Disabled app.commandLine.appendSwitch('disable-gpu-program-cache'); - 🚫 Prevents compiled GPU program caching issues - 🔒 Eliminates permission-related failures ### 3️⃣ Startup Cache Clearing session.defaultSession.clearCache() .then(() => console.log('[main] Cleared cache on startup')) .catch((err) => console.warn('[main] Failed to clear cache:', err)); - 🧹 Clears stale session cache on initialization - 🔧 Prevents errors from corrupted cache artifacts - ⚠️ Includes error handling for robustness --- ## 📝 Technical Changes ### Files Modified | File | Changes | |------|---------| | apps/frontend/src/main/index.ts | +13 lines (cache fixes) | ### Platform Gating ✅ **Windows Only** (process.platform === 'win32') ✅ macOS & Linux behavior unchanged --- ## 🎯 Impact Assessment | Aspect | Status | Details | |--------|--------|---------| | 🎮 **GPU Acceleration** | ✅ **PRESERVED** | Hardware rendering fully functional | | 🤖 **Agent Functionality** | ✅ **RESTORED** | Coding phase now works on Windows | | 🖥️ **Console Errors** | ✅ **ELIMINATED** | Clean startup on all Windows systems | | ⚡ **Performance** | ✅ **NO IMPACT** | Typical usage unaffected | | 🔙 **Compatibility** | ✅ **MAINTAINED** | No breaking changes | --- ## 🧪 Testing ### Test Environments | Platform | Antivirus | Result | |----------|-----------|--------| | Windows 10 | Windows Defender | ✅ Pass | | Windows 11 | Real-time scanning | ✅ Pass | ### Test Scenarios ✅ Application starts without cache errors ✅ Agent initialization completes successfully ✅ Coding phase executes without GPU failures ✅ GPU acceleration functional (hardware rendering active) --- ## 📦 Meta Information | Field | Value | |-------|-------| | 📍 **Component** | apps/frontend/src/main/index.ts | | 🪟 **Platform** | Windows (win32) - platform-gated | | 🔥 **Type** | Hotfix (critical functionality restoration) | --- ## 🔄 Backwards Compatibility | Check | Status | |-------|--------| | Breaking Changes | ❌ None | | User Data Migration | ❌ Not required | | Settings Impact | ❌ Unaffected | | Workflow Changes | ❌ None required | --- *This hotfix restores critical functionality for Windows users while maintaining full compatibility with macOS and Linux platforms. GPU acceleration remains fully functional — only disk-based caching is disabled.* Co-authored-by: sniggl <[email protected]> * ci(release): add CHANGELOG.md validation and fix release workflow The release workflow was failing with "GitHub Releases requires a tag" when triggered via workflow_dispatch because no tag existed. Changes: - prepare-release.yml: Validates CHANGELOG.md has entry for version BEFORE creating tag (fails early with clear error message) - release.yml: Uses CHANGELOG.md content instead of release-drafter for release notes; fixes workflow_dispatch to be dry-run only - bump-version.js: Warns if CHANGELOG.md missing entry for new version - RELEASE.md: Updated documentation for new changelog-first workflow This ensures releases are only created when CHANGELOG.md is properly updated, preventing incomplete releases and giving better release notes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(merge): handle Windows CRLF line endings in regex fallback The merge conflict layer was failing on Windows when tree-sitter was unavailable. The regex-based fallback used split("\n") which doesn't handle CRLF line endings, and findall() returned tuples for JS/TS patterns breaking function detection. Changes: - Normalize line endings (CRLF → LF) before parsing in regex_analyzer.py - Use splitlines() instead of split("\n") in file_merger.py - Fix tuple extraction from findall() for JS/TS function patterns - Normalize line endings before tree-sitter parsing for consistent byte positions All 111 merge tests pass. These changes are cross-platform safe and maintain compatibility with macOS and Linux. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * 2.7.2 release --------- Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: sniggl <[email protected]> Co-authored-by: sniggl <[email protected]> * ci: force workflow re-index for tag trigger * fix(ci): fix YAML syntax error in release.yml multiline string * docs: update stable download links to v2.7.2 (#579) * Fix Windows UTF-8 encoding errors in spec file reading (#744) * Fix Windows UTF-8 encoding errors in spec file reading Fixes UnicodeDecodeError on Windows when reading spec files with non-ASCII characters. Python's read_text() defaults to cp1252 on Windows instead of UTF-8, causing crashes when spec.md contains Unicode characters. Changes: - spec_document_validator.py: Add encoding='utf-8' to spec.md reading - compaction.py: Add encoding='utf-8' to phase output file reading - validation_strategy.py: Add encoding='utf-8' to requirements/pyproject reading - agent_runner.py: Add encoding='utf-8' to prompt file reading Impact: - Fixes spec generation crashes on Windows with Unicode content - No impact on Unix/Mac (already default to UTF-8) - Prevents charmap codec errors during spec validation Tested on Windows 10 with Auto-Claude v2.7.3 * Add error handling for file read operations Implements Gemini Code Assist review suggestions to improve robustness: 1. spec_document_validator.py: - Wrap read_text() in try-except to handle corrupted UTF-8 files - Provide clear error message when spec.md is unreadable 2. validation_strategy.py: - Silently skip unreadable requirements.txt/pyproject.toml files - Prevents crashes during project type detection This defensive programming prevents crashes from: - Corrupted or invalid UTF-8 files - Permission errors (OSError) - Other file system issues Addresses code review feedback from @gemini-code-assist * Fix Windows UTF-8 encoding errors in project analyzer - Add encoding='utf-8' to file reads in analyzer.py (load/save profile) - Add encoding='utf-8' to file reads in config_parser.py (read_json, read_text) - Add encoding='utf-8' to file reads in stack_detector.py (YAML files) This fixes the 'charmap' codec error that was blocking roadmap generation on Windows: Warning: Could not load security profile: 'charmap' codec can't decode byte 0x8d Same root cause as spec file reading - Windows defaults to cp1252 instead of UTF-8. * Revert "Fix Windows UTF-8 encoding errors in project analyzer" This reverts commit 41deed0aa388d8599371db16f822d9278464f09b. --------- Co-authored-by: TamerineSky <[email protected]> * Revert "Fix Windows UTF-8 encoding errors in spec file reading (#744)" (#770) This reverts commit a01634d54809e9f4f1d6507d5bcbf55fdff68981. * feat: Add workspace mode preference (ISOLATED/DIRECT) - Frontend: Add workspace mode setting in General Settings UI with dropdown - Frontend: Map workspaceMode TypeScript field to WORKSPACE_MODE env variable - Frontend: Read WORKSPACE_MODE from .env back to workspaceMode for UI - Backend: Read WORKSPACE_MODE from project .env in choose_workspace() - Backend: Respect user preference before defaulting to safety checks This allows users to configure their preferred workspace mode (isolated worktree vs direct) through the UI, persisted to .auto-claude/.env, and respected by the backend agent. * feat: Add Playwright screenshot collection after QA sessions Backend changes: - Add collect_qa_screenshots() to scan for recent .png files after QA - Add save_qa_screenshots_to_plan() to save paths to implementation_plan.json - Call screenshot collection in QA reviewer after session completes - Screenshots are saved to qa_signoff.screenshots field Frontend changes: - Add screenshots?: string[] field to QAReport TypeScript type This enables capturing and storing Playwright screenshots during QA validation. Next step: Map screenshots to Task.qaReport and display in UI. * feat: Map qa_signoff to Task.qaReport with screenshots - Import QAReport type in project-store - Extract qa_signoff from implementation_plan.json when loading tasks - Map qa_signoff.status to qaReport.status (approved -> passed, rejected -> failed) - Map qa_signoff.issues_found to qaReport.issues array - Map qa_signoff.screenshots to qaReport.screenshots array - Convert timestamp to Date object This enables frontend to display QA results and Playwright screenshots in task UI. * feat: Enable Claude to see and verify Playwright screenshots during QA CRITICAL CHANGE: QA agent can now visually verify screenshots Backend changes: - playwright/executor.py: Return base64-encoded screenshot in tool response - playwright/sdk_integration.py: Format screenshot as image content block for Claude - qa_reviewer.md prompt: Explicit instructions to ANALYZE visual content How it works now: 1. QA agent uses playwright_screenshot tool 2. Tool returns base64-encoded image + metadata 3. SDK formats it as image content block 4. Claude SEES the screenshot and can analyze visual content 5. If Claude detects visual bugs → reports them as issues 6. QA fixer fixes the issues 7. Loop continues until approved Before this change: Screenshots were taken but Claude never saw them After this change: Claude can verify UI matches acceptance criteria visually This closes the verification loop - screenshots are not just captured for later viewing, but actively analyzed by the QA agent during validation. * feat: add screenshot support to QA feedback Users can now paste/drag screenshots in QA feedback textarea to show visual problems. Frontend changes: - QAFeedbackSection: Added image paste/drag-drop with useImageUpload hook - Shows image previews with thumbnails and count indicator - Visual feedback on paste success (green border flash) - Added feedbackImages state to useTaskDetail hook - Updated submitReview to accept ImageAttachment[] parameter Backend changes: - IPC handler saves images to qa-feedback-screenshots/ directory - References screenshots in QA_FIX_REQUEST.md with instructions - QA fixer loads and displays images as visual content blocks - Claude sees screenshots and can analyze visual problems Complete flow: 1. User pastes screenshot (Cmd+V or drag-drop) 2. Image saved as base64 ImageAttachment 3. On "Request Changes", sent via IPC to backend 4. Backend saves to qa-feedback-screenshots/ 5. QA_FIX_REQUEST.md lists screenshot paths 6. QA fixer loads and sends as image blocks to Claude 7. Claude sees screenshots visually and fixes problems This mirrors the Playwright screenshot pattern - Claude gets visual context, not just file paths. * security: Add input validation and sanitization for screenshot uploads Address security concerns from code review: 1. Path Traversal Prevention: - Sanitize filenames with path.basename() - Validate only safe characters (alphanumeric, dash, underscore, dot) - Resolve and validate paths before reading screenshots 2. Resource Limits: - Enforce 10MB max size per image - Log rejected files with size/reason 3. Separation of Concerns: - Exclude user feedback screenshots from agent screenshot collection - Only collect Playwright/agent-generated screenshots in collect_qa_screenshots() 4. UX Improvement: - Allow 'Request Changes' with images only (no text required) These changes prevent: - Directory traversal attacks (../../etc/passwd) - Arbitrary file overwrites - Disk exhaustion via unbounded uploads - Mixing user and agent screenshots * fix: Include user feedback text with screenshots for full context Extract feedback text from QA_FIX_REQUEST.md and include it in the content blocks sent to Claude along with the screenshots. This ensures the QA fixer sees both the text description and visual context together. Changes: - Modified load_qa_feedback_screenshots() to return tuple of (feedback_text, image_blocks) - Extract feedback section from markdown with regex - Include feedback text in prompt before image blocks - Provide clear context that screenshots show visual problems * feat: Update app icon to new design - Converted WebP images from appiconset to PNG format - Generated new icon.icns using iconutil - Updated icon.png (1024x1024) and icon-256.png - Packaged and verified new icon in Mac build * feat: Display QA report screenshots in task detail UI - Created QAReportSection component to display Playwright screenshots - Shows screenshots in grid with zoom functionality - Positioned between workspace status and QA feedback section - Screenshots are clickable to view full-size in dialog - Only displays when QA has run and screenshots exist * feat: Add Playwright headless/headed mode toggle - Added playwrightHeadless setting to ProjectEnvConfig (default: true) - Backend: Read PLAYWRIGHT_HEADLESS env var in executor (default: true) - Frontend: Added UI toggle in project settings Workspace section - .env file integration for PLAYWRIGHT_HEADLESS setting - Allows users to run Playwright in headed mode for debugging * fix: Add ImageAttachment parameter to submitReview type - Added ImageAttachment import to ipc.ts - Updated submitReview signature to include images parameter - Fixes TypeScript compilation error * feat: implement automatic MCP tool discovery via SERVER_TOOL_REGISTRY Replaced hardcoded if/elif chains in _get_mcp_tools_for_servers() with a registry pattern for automatic tool discovery. Changes: - Added SERVER_TOOL_REGISTRY dict mapping server names to tool lists - _get_mcp_tools_for_servers() now uses simple dict lookup - Exported SERVER_TOOL_REGISTRY for external use - Added ADDING_MCP_SERVERS.md documentation Benefits: - Adding new MCP servers requires just 2 steps (define tools, add to registry) - No more manual if/elif modification for each new server - DRY principle - tool mappings defined once - Easier maintenance and debugging All existing servers (context7, linear, graphiti, electron, puppeteer, playwright) work unchanged. Verified via comprehensive tests. Fixes the original issue where Playwright tools weren't automatically available when the server was added to agent configs. * fix(ACS-175): Resolve integrations freeze and improve rate limit handling (#839) * fix(ACS-175): Resolve integrations freeze and improve rate limit handling * fix(i18n): replace hardcoded toast strings with translation keys Addresses CodeRabbit review feedback on PR #839: - OAuthStep.tsx: use t() for all toast messages - RateLimitModal.tsx: use t() for toast messages - SDKRateLimitModal.tsx: add useTranslation hook, use t() for toasts - Add toast translation keys to en/fr onboarding.json and common.json * fix: replace console.log with debugLog in IntegrationSettings Addresses CodeRabbit review feedback - use project's debug logging utility for consistent and toggle-able logging in production. * fix: replace console.log with debugLog in main process files Addresses Auto-Claude PR Review feedback: - terminal-handlers.ts: 14 console.log → debugLog - pty-manager.ts: 10 console.log → debugLog/debugError - terminal-manager.ts: 4 console.log → debugLog/debugError Also fixes: - Extract magic numbers to CHUNKED_WRITE_THRESHOLD/CHUNK_SIZE constants - Add terminal validity check before chunked writes - Consistent error handling (no rethrow for fire-and-forget semantics) * fix: address Auto Claude PR review findings - console.error→debugError + i18n - Replace 8 remaining console.error/warn calls with debugError/debugLog: - terminal-handlers.ts: lines 59, 426, 660, 671 - terminal-manager.ts: lines 88, 320 - pty-manager.ts: lines 88, 141 - Fixed duplicate logging in exception handler - Add comprehensive i18n for SDKRateLimitModal.tsx (~20 strings): - Added rateLimit.sdk.* keys with swap notifications, buttons, labels - EN + FR translations in common.json - Add comprehensive i18n for OAuthStep.tsx (~15 strings): - Added oauth.badges.*, oauth.buttons.*, oauth.labels.* keys - EN + FR translations in onboarding.json All MEDIUM severity findings resolved except race condition (deferred). * fix: address Auto Claude PR review findings - race condition + console.error - Fix race condition in chunked PTY writes by serializing writes per terminal using Promise chaining (prevents interleaving of concurrent large writes) - Fix missing 't' in useEffect dependency array in OAuthStep.tsx - Convert all remaining console.error calls to debugError for consistency: - IntegrationSettings.tsx (9 occurrences) - RateLimitModal.tsx (3 occurrences) - SDKRateLimitModal.tsx (4 occurrences) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: Black Circle Sentinel <[email protected]> --------- Signed-off-by: Black Circle Sentinel <[email protected]> Co-authored-by: Andy <[email protected]> Co-authored-by: Adam Slaker <[email protected]> Co-authored-by: Claude Opus 4.5 <[email protected]> * fix: Playwright headed mode by default + enforce relative screenshot paths Changes: 1. Headless mode: - Changed default from headless=true to headless=false (HEADED/visible) - Browser now opens visibly by default for debugging - Set PLAYWRIGHT_HEADLESS=true in .env to run headless 2. Screenshot paths: - Added prominent warnings in QA prompt about using relative paths - Updated tool description to enforce relative paths - Updated tool input schema description with clear warning - Relative paths are saved in spec dir for persistence - Absolute paths in /tmp are lost after cleanup Why: - User reported browser wasn't visible during QA testing - Screenshto zapisywał się w /tmp i znikał po cleanup - Agent needs clear instruction to use relative paths only Now: - Browser opens visibly for easy debugging - Screenshots persist in spec directory - Clear warnings prevent agent from using /tmp paths * fix: Load PLAYWRIGHT_HEADLESS from .env dynamically in QA loop Problem: When user changes PLAYWRIGHT_HEADLESS in UI, backend doesn't see it because os.environ is static (loaded once at process start). Solution: 1. Added PLAYWRIGHT_HEADLESS to mcp_keys in load_project_mcp_config() 2. QA loop now loads mcp_config and sets os.environ["PLAYWRIGHT_HEADLESS"] BEFORE creating Playwright browser 3. This allows UI changes to take effect immediately without backend restart Flow: - User changes setting in UI → saved to .auto-claude/.env - QA loop starts → loads .env file dynamically - Sets os.environ["PLAYWRIGHT_HEADLESS"] = value from .env - Playwright executor reads os.environ → uses fresh value Result: UI changes to headed/headless mode now work immediately! * docs: Add AUTO-CLAUDE BUILT-IN CAPABILITIES section to planner prompt Problem: Planner doesn't know about Auto-Claude's built-in tools and creates unnecessary subtasks to install them (e.g., 'npx playwright'). Solution: Added prominent section at the top of planner.md explaining: - Playwright: Already installed, available to QA agents, don't use npx - Context7: Built-in docs lookup - Graphiti: Built-in knowledge graph memory - Linear: Built-in PM integration Also updated examples: - Changed 'npx playwright test' → 'QA agent with built-in Playwright' - Updated e2e_tests example to reference built-in Playwright Result: Planner now understands these tools exist and focuses subtasks on actual feature implementation, not on setting up Auto-Claude infrastructure. This prevents planner from: - Creating 'install Playwright' subtasks - Trying to use 'npx playwright' commands - Assuming tools need to be configured in the project * fix: Add mcp__playwright__ prefix to Playwright tool names Problem: Agent tries to use 'mcp__playwright__playwright_navigate' but gets permission error because PLAYWRIGHT_TOOLS had names without MCP prefix. Root cause: Playwright was converted to SDK MCP server (commit dd108b9) but tool names in PLAYWRIGHT_TOOLS weren't updated to include the mcp__playwright__ prefix that SDK automatically adds. Comparison: - Context7: mcp__context7__resolve-library-id ✓ - Linear: mcp__linear-server__list_teams ✓ - Playwright: playwright_navigate ✗ (missing prefix!) Solution: Updated all PLAYWRIGHT_TOOLS to include mcp__playwright__ prefix: - playwright_navigate → mcp__playwright__playwright_navigate - playwright_screenshot → mcp__playwright__playwright_screenshot - etc. This matches the naming convention used by all other MCP servers. Result: QA agent can now actually use Playwright tools without permission errors. * feat: Use wildcard MCP permissions for automatic tool discovery Problem: Manual tool name mapping required in two places: 1. TOOL_NAME constants (e.g., PLAYWRIGHT_TOOLS list) 2. Security permissions (hardcoded if/else for each server) This caused permission errors when: - Tool names didn't match exactly - New tools were added to MCP servers - Servers were renamed or updated Solution: Wildcard MCP permissions! Before (MANUAL): - List every tool: CONTEXT7_TOOLS, LINEAR_TOOLS, etc. - Add permissions for each tool individually - Update both places when adding new tools After (AUTOMATIC): - If server in required_servers → grant mcp__<server>__*(*) permission - All tools from that server work immediately - No manual tool name listing needed Changes: 1. Replaced individual tool permissions with wildcard pattern 2. Removed browser_tools_permissions logic (now automatic) 3. Removed unused imports (CONTEXT7_TOOLS, LINEAR_TOOLS, etc.) Example permissions generated: - mcp__context7__*(*) → all Context7 tools - mcp__linear__*(*) → all Linear tools - mcp__playwright__*(*) → all Playwright tools - mcp__graphiti__*(*) → all Graphiti tools Result: - ✅ Permission errors eliminated - ✅ New MCP tools work immediately - ✅ Cleaner code (removed 30+ lines of manual mapping) - ✅ True automatic tool discovery * fix(frontend): strip ANSI escape codes from roadmap/ideation progress messages (ACS-219) (#933) * fix(frontend): strip ANSI escape codes from roadmap/ideation progress messages (ACS-219) - Create ansi-sanitizer.ts utility with stripAnsiCodes() function - Apply to roadmap and ideation progress handlers in agent-queue.ts - Add 34 comprehensive test cases covering ANSI escape patterns - Fixes raw escape codes (e.g., \x1b[90m) displaying in UI * fix(frontend): extract first line before truncating roadmap progress messages Make roadmap progress message construction consistent with ideation by extracting the first line (split by newline) before applying the 200 character truncation. This ensures multi-line log output doesn't display partial lines in the UI. * refactor(frontend): extract repeated status message formatting to helper Add formatStatusMessage() helper function to centralize the sanitize+truncate pattern used in progress message handlers. This improves maintainability by: - Adding STATUS_MESSAGE_MAX_LENGTH constant (200) - Encapsulating ANSI stripping, line extraction, and truncation - Replacing 4 duplicated call sites with single helper Refactors lines 442, 461, 704, 718 to use formatStatusMessage(log). --------- Co-authored-by: StillKnotKnown <[email protected]> * fix(frontend): prevent "Render frame was disposed" crash (ACS-211) (#918) * fix(frontend): prevent "Render frame was disposed" crash (ACS-211) Add safeSendToRenderer helper that validates frame state before IPC sends. Prevents app crash when renderer frames are disposed during heavy agent output. Fixes #211 Signed-off-by: StillKnotKnown <[email protected]> * refactor(utils): replace setInterval with timestamp-based cooldown, add \r\n support - Replace setInterval-based cooldown with timestamp Map approach to avoid timer leaks - Add isWithinCooldown() and recordWarning() helper functions - Optionally prune old entries when Map exceeds 100 entries - Update parseEnvFile to handle Windows \r\n line endings with /\r?\n/ regex Signed-off-by: StillKnotKnown <[email protected]> * refactor(utils): enforce hard cap of 100 entries in pruning logic - First remove expired entries (outside cooldown period) - If still over 100 entries, remove oldest by insertion order - Ensures Map never exceeds 100 entries even during heavy load Signed-off-by: StillKnotKnown <[email protected]> * test: fix module-level state issue in pruning tests - Add _clearWarnTimestampsForTest() helper to clear module-level Map - Call clear in parent beforeEach to ensure clean state for each test - Simplify pruning tests to avoid complex cooldown timing issues - All 28 tests now pass * fix: update generation-handlers.ts to use safeSendToRenderer Addresses finding NEW-003 from follow-up review: - Import safeSendToRenderer from '../utils' - Replace all 5 direct webContents.send() calls with safeSendToRenderer - Add getMainWindow wrapper for each function This ensures ideation generation IPC messages are protected from "Render frame was disposed" crashes. Related: ACS-211 --------- Signed-off-by: StillKnotKnown <[email protected]> Co-authored-by: StillKnotKnown <[email protected]> * fix(core): implement atomic JSON writes to prevent file corruption (ACS-209) (#915) * fix(core): implement atomic JSON writes to prevent file corruption (ACS-209) - Add write_json_atomic() utility using temp file + os.replace() - Add async_save() to ImplementationPlan for non-blocking I/O - Update planner.py to use async_save() in async context - Add 'error' status to TaskStatus for corrupted files - Enhance ProjectStore error handling to surface parse errors - Add recovery CLI utility (cli/recovery.py) for detecting/fixing corrupted files This fixes JSON parse errors that caused tasks to be silently skipped when implementation_plan.json was corrupted during crashes/interrupts. * fix(i18n): add error column to task status constants and translations - Add 'error' to TASK_STATUS_COLUMNS array - Add 'error' label and color to TASK_STATUS_LABELS and TASK_STATUS_COLORS - Add 'columns.error' translation key to en/tasks.json and fr/tasks.json This fixes TypeScript errors in KanbanBoard.tsx where the Record type was missing the 'error' status that was added to TaskStatus type. * refactor: improve code quality and add i18n support for error messages Backend improvements: - Fix duplicate JSON check in recovery.py (remove redundant plan_file check) - Update find_specs_dir return type to Path (always returns valid path) - Replace deprecated asyncio.get_event_loop() with asyncio.get_running_loop() - Use functools.partial for cleaner async_save implementation Frontend improvements: - Add TaskErrorInfo type for structured error information - Update project-store.ts to use errorInfo for i18n-compatible error messages - Tighten typing of TASK_STATUS_LABELS and TASK_STATUS_COLORS to use TaskStatusColumn - Add error column to KanbanBoard grouped tasks initialization - Add en/fr errors.json translation files for parse error messages * docs: add errors.json to i18n translation namespaces - Document new errors.json namespace for error messages - Add example showing interpolation/substitution pattern for dynamic error content * refactor: typing improvements, i18n fixes, and error UX enhancements Backend typing: - Add explicit return type hint (-> None) to main() in recovery.py - Add explicit return type hint (-> None) to async_save() in plan.py - Make recovery.py exit with code 1 when corrupted files are detected Error handling improvements: - Include specId in errorInfo meta for better error context - Cap error message length at 500 characters to prevent bloat - Update error translation keys to include specId substitution Frontend improvements: - Conditionally add reviewReason/errorInfo to task objects only when defined - Add 'error' case to KanbanBoard empty state with AlertCircle icon - Fix hardcoded "Refreshing..."/"Refresh Tasks" strings to use i18n i18n additions: - Add emptyError/emptyErrorHint to en/fr tasks.json - Add refreshing/refreshTasks to en/fr translation files * refactor: code quality improvements - Remove error truncation in recovery.py (show full error for debugging) - Extract duplicate timestamp/status update logic to _update_timestamps_and_status() helper - Fix MD031 in CLAUDE.md (add blank line before fenced code block) * refactor: code quality improvements - Remove error truncation in recovery.py (show full error for debugging) - Extract duplicate timestamp/status update logic to _update_timestamps_and_status() helper - Fix MD031 in CLAUDE.md (add blank line before fenced code block) * refactor: naming and typing improvements - Update docstring examples in recovery.py (--fix -> --delete) - Rename delete_corrupted_file to backup_corrupted_file for clarity - Add return type annotation -> None to save() method * fix: add error status handling to TaskCard - Add 'error' case to getStatusBadgeVariant() returning 'destructive' - Add 'error' case to getStatusLabel() returning t('columns.error') - Start/stop buttons already exclude error tasks (backlog/in_progress only) * fix: address PR review feedback Backend changes: - recovery.py: Change [DELETE] to [BACKUP] in output message to match operation - recovery.py: Replace startswith with is_relative_to for proper path validation - recovery.py: Handle existing .corrupted backup files with unique timestamp suffix - file_utils.py: Add Iterator[IO[str]] return type annotation to atomic_write Frontend changes: - KanbanBoard.tsx: Use column-error CSS class instead of inline border-t-destructive - globals.css: Add .column-error rule with destructive color for consistency - tasks.json: Add top-level refreshTasks translation key for en/fr locales * fix: address follow-up review findings Backend changes: - file_utils.py: Handle binary mode correctly (encoding=None for 'b' in mode) - file_utils.py: Change return type to Iterator[IO] for broader type support - file_utils.py: Add docstring note about binary mode support - plan.py: Fix async_save to restore state on write failure (captures timestamps) Frontend changes: - project-store.ts: Add 'error' to statusMap to preserve error status - project-store.ts: Add early return for 'error' status like 'done'/'pr_created' - task-store.ts: Allow recovery from error status to backlog/in_progress - task-store.ts: Allow transitions to error without completion validation * fix: address follow-up review findings Backend changes: - file_utils.py: Handle binary mode correctly (encoding=None for 'b' in mode) - file_utils.py: Change return type to Iterator[IO] for broader type support - file_utils.py: Add docstring note about binary mode support - plan.py: Fix async_save to restore state on write failure (captures timestamps) Frontend changes: - project-store.ts: Add 'error' to statusMap to preserve error status - project-store.ts: Add early return for 'error' status like 'done'/'pr_created' - task-store.ts: Allow recovery from error status to backlog/in_progress - task-store.ts: Allow transitions to error without completion validation * fix: address third follow-up review findings Backend changes: - recovery.py: Change spec_dir.glob to spec_dir.rglob for recursive JSON scan - file_utils.py: Fix fd leak when os.fdopen raises (close fd and unlink tmp_path) - plan.py: Capture full state with to_dict() for robust rollback in async_save * fix: address final review quality suggestions Backend changes: - plan.py: Add NOTE comment about rollback fields maintenance - file_utils.py: Add logging.warning for temp file cleanup failures * fix: address final review quality suggestions Backend changes: - plan.py: Add NOTE comment about rollback fields maintenance - file_utils.py: Add logging.warning for temp file cleanup failures * fix: address final review quality suggestions Backend changes: - plan.py: Add NOTE comment about rollback fields maintenance - file_utils.py: Add logging.warning for temp file cleanup failures * fix: include stack trace in temp file cleanup failure logging Add exc_info=True to logging.warning call when temp file cleanup fails. This captures the full stack trace for better postmortem debugging of orphaned temp files. --------- Co-authored-by: StillKnotKnown <[email protected]> Co-authored-by: Andy <[email protected]> * fix(workspace): auto-rebase spec branch when behind before merge (#945) (#946) * fix(workspace): auto-rebase spec branch when behind before merge (ACS-224) When the "Merge with AI" button is clicked and the spec branch is behind the target branch, the system now automatically rebases before attempting to merge. Previously, the merge preview correctly detected the "behind" state but the actual merge process would fail with conflict errors. Changes: - Enhanced _check_git_conflicts() to detect needs_rebase and commits_behind - Added _rebase_spec_branch() to automatically rebase spec branch - Integrated rebase logic into _try_smart_merge_inner() before conflict resolution - Added 10 comprehensive tests for rebase detection and execution * fix(workspace): correct rebase invocation and use run_git helper Fixes issues in _rebase_spec_branch function: - Use run_git() instead of subprocess.run for allowlist compliance - Fix fragile git rebase arg order - use standard invocation - Remove misleading --strategy-option=theirs (not actually used) - Return False when conflicts abort (no ref movement occurred) - Verify branch actually moved after successful rebase Also updates test to check both .git/rebase-merge and .git/rebase-apply directories for robust git version compatibility. * fix: address PR review findings for rebase function Fixes 7 issues identified in PR review: HIGH: - Save and restore original branch after rebase (prevents leaving repo on spec branch) MEDIUM: - Return True when branch is already up-to-date (success, not failure) - Fix conflict detection to parse git status codes properly (not 'U' substring) - Check rebase abort result for inconsistent state LOW: - Remove unused variables git_dir and git_backup from test - Deduplicate git_conflicts.get('commits_behind', 0) call - Rename test to accurately describe what it tests * fix: address follow-up PR review findings Additional fixes from code review: workspace.py: - Remove duplicate "UD" from conflict status codes tuple - Add returncode check for original_branch_result with proper validation - Guard finally block restore with original_branch validity check - Replace subprocess.run with run_git for rev-list call - Add error logging when rev-list fails test_workspace.py: - Remove duplicate test test_rebase_handles_nonexistent_branch_gracefully (redundant with test_rebase_spec_branch_invalid_branch) - Strengthen merge assertion from "is not None" to "is True" * fix: replace remaining subprocess.run with run_git and add comprehensive tests workspace.py: - Replace all subprocess.run calls in _check_git_conflicts with run_git for consistent error handling, timeout handling, and centralized logging test_workspace.py: - Add test_rebase_spec_branch_already_up_to_date to verify function returns True when spec branch is already current - Add test_check_git_conflicts_handles_detached_head to verify graceful handling of detached HEAD state - Add test_check_git_conflicts_handles_corrupted_repo to verify graceful handling of corrupted .git directory with proper cleanup - Fix test_check_git_conflicts_no_commits_behind to checkout main before assertion (was comparing spec to itself, always returning 0) Test count: 42 -> 45 (+3 new tests) * fix: remove unused tempfile import from test * fix(workspace): final PR review fixes for ACS-224 rebase functionality This commit addresses the final batch of PR review findings for the rebase functionality added in ACS-224. All 45 tests pass. Changes: - NEW-001: Added branch restoration failure tracking and logging in finally block (noted limitation: cannot return False from finally block) - NEW-002: Added abort_failed tracking and checks before returning True to propagate abort failures to caller - NEW-004: Added state verification assertions to test_rebase_spec_branch_invalid_branch to verify current branch, no rebase state directories, and clean git status - LOGIC-002: Wrapped int() conversion in try-except to handle malformed rev-list output gracefully - LOGIC-003: Simplified redundant condition from checking both needs_rebase and commits_behind > 0 to just checking needs_rebase (which implies > 0) Files modified: - apps/backend/core/workspace.py: Added abort_failed tracking and error handling - tests/test_workspace.py: Added repo state verification assertions Related: ACS-224 * fix(workspace): fix CodeQL warnings for unreachable code and unused variable CodeQL detected unreachable code and unused variable issues in the rebase function. The abort_failed flag was only set when rebase failed, but was only checked in the success path (making it unreachable). Fixed by: - Removing abort_failed variable and its unreachable checks - Immediately returning False when abort fails (cannot safely continue) - Simplifying the finally block comment to reflect actual behavior All 45 tests pass. Related: ACS-224 --------- Co-authored-by: StillKnotKnown <[email protected]> * New/relase (#949) * chore: bump version to 2.7.3 * chore: update CHANGELOG for version 2.7.3, highlighting new features, improvements, and bug fixes --------- Co-authored-by: Test User <[email protected]> * fix: address CodeQL and PR review findings - Fix shell command injection vulnerability in cli-tool-manager.ts by using cmd.exe with argument array instead of string interpolation - Fix unused variables in test files (PRDetail.test.tsx, ReviewStatusTree.test.tsx) - Remove unused 'issues' destructuring in GitLabIssues.tsx - Fix unused 'merge_base' variable in workspace.py - Remove INVESTIGATION.md (temporary investigation document) Co-Authored-By: Claude Opus 4.5 <[email protected]> * Release v2.7.3 (#951) * docs: Add Git Flow branching strategy to CONTRIBUTING.md - Add comprehensive branching strategy documentation - Explain main, develop, feature, fix, release, and hotfix branches - Clarify that all PRs should target develop (not main) - Add release process documentation for maintainers - Update PR process to branch from develop - Expand table of contents with new sections * Feature/apps restructure v2.7.2 (#138) * refactor: restructure project to Apps/frontend and Apps/backend - Move auto-claude-ui to Apps/frontend with feature-based architecture - Move auto-claude to Apps/backend - Switch from pnpm to npm for frontend - Update Node.js requirement to v24.12.0 LTS - Add pre-commit hooks for lint, typecheck, and security audit - Add commit-msg hook for conventional commits - Fix CommonJS compatibility issues (postcss.config, postinstall scripts) - Update README with comprehensive setup and contribution guidelines - Configure ESLint to ignore .cjs files - 0 npm vulnerabilities Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat(refactor): clean code and move to npm * feat(refactor): clean code and move to npm * chore: update to v2.7.0, remove Docker deps (LadybugDB is embedded) * feat: v2.8.0 - update workflows and configs for Apps/ structure, npm * fix: resolve Python lint errors (F401, I001) * fix: update test paths for Apps/backend structure * fix: add missing facade files and update paths for Apps/backend structure - Fix ruff lint error I001 in auto_claude_tools.py - Create missing facade files to match upstream (agent, ci_discovery, critique, etc.) - Update test paths from auto-claude/ to Apps/backend/ - Update .pre-commit-config.yaml paths for Apps/ structure - Add pytest to pre-commit hooks (skip slow/integration/Windows-incompatible tests) - Fix Unicode encoding in test_agent_architecture.py for Windows Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * feat: improve readme * fix: new path * fix: correct release workflow and docs for Apps/ restructure - Fix ARM64 macOS build: pnpm → npm, auto-claude-ui → Apps/frontend - Fix artifact upload paths in release.yml - Update Node.js version to 24 for consistency - Update CLI-USAGE.md with Apps/backend paths - Update RELEASE.md with Apps/frontend/package.json paths 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * refactor: rename Apps/ to apps/ and fix backend path resolution - Rename Apps/ folder to apps/ for consistency with JS/Node conventions - Update all path references across CI/CD workflows, docs, and config files - Fix frontend Python path resolver to look for 'backend' instead of 'auto-claude' - Update path-resolver.ts to correctly find apps/backend in development mode This completes the Apps restructure from PR #122 and prepares for v2.8.0 release. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(electron): correct preload script path from .js to .mjs electron-vite builds the preload script as ESM (index.mjs) but the main process was looking for CommonJS (index.js). This caused the preload to fail silently, making the app fall back to browser mock mode with fake data and non-functional IPC handlers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * - Introduced `dev:debug` script to enable debugging during development. - Added `dev:mcp` script for running the frontend in MCP mode. These enhancements streamline the development process for frontend developers. * refactor(memory): make Graphiti memory mandatory and remove Docker dependency Memory is now a core component of Auto Claude rather than optional: - Python 3.12+ is required for the backend (not just memory layer) - Graphiti is enabled by default in .env.example - Removed all FalkorDB/Docker references (migrated to embedded LadybugDB) - Deleted guides/DOCKER-SETUP.md and docker-handlers.ts - Updated onboarding UI to remove "optional" language - Updated all documentation to reflect LadybugDB architecture 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * feat: add cross-platform Windows support for npm scripts - Add scripts/install-backend.js for cross-platform Python venv setup - Auto-detects Python 3.12 (py -3.12 on Windows, python3.12 on Unix) - Handles platform-specific venv paths - Add scripts/test-backend.js for cross-platform pytest execution - Update package.json to use Node.js scripts instead of shell commands - Update CONTRIBUTING.md with correct paths and instructions: - apps/backend/ and apps/frontend/ paths - Python 3.12 requirement (memory system now required) - Platform-specific install commands (winget, brew, apt) - npm instead of pnpm - Quick Start section with npm run install:all 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * remove doc * fix(frontend): correct Ollama detector script path after apps restructure The Ollama status check was failing because memory-handlers.ts was looking for ollama_model_detector.py at auto-claude/ but the script is now at apps/backend/ after the directory restructure. This caused "Ollama not running" to display even when Ollama was actually running and accessible. * chore: bump version to 2.7.2 Downgrade version from 2.8.0 to 2.7.2 as the Apps/ restructure is better suited as a patch release rather than a minor release. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * chore: update package-lock.json for Windows compatibility 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * docs(contributing): add hotfix workflow and update paths for apps/ structure Add Git Flow hotfix workflow documentation with step-by-step guide and ASCII diagram showing the branching strategy. Update all paths from auto-claude/auto-claude-ui to apps/backend/apps/frontend and migrate package manager references from pnpm to npm to match the new project structure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(ci): remove duplicate ARM64 build from Intel runner The Intel runner was building both x64 and arm64 architectures, while a separate ARM64 runner also builds arm64 natively. This caused duplicate ARM64 builds, wasting CI resources. Now each runner builds only its native architecture: - Intel runner: x64 only - ARM64 runner: arm64 only 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Alex Madera <[email protected]> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.5 <[email protected]> * Feat: Ollama download progress tracking with new apps structure (#141) * feat(ollama): add real-time download progress tracking for model downloads Implement comprehensive download progress tracking with: - NDJSON parsing for streaming progress data from Ollama API - Real-time speed calculation (MB/s, KB/s, B/s) with useRef for delta tracking - Time remaining estimation based on download speed - Animated progress bars in OllamaModelSelector component - IPC event streaming from main process to renderer - Proper listener management with cleanup functions Changes: - memory-handlers.ts: Parse NDJSON from Ollama stderr, emit progress events - OllamaModelSelector.tsx: Display progress bars with speed and time remaining - project-api.ts: Implement onDownloadProgress listener with cleanup - ipc.ts types: Define onDownloadProgress listener interface - infrastructure-mock.ts: Add mock implementation for browser testing This allows users to see real-time feedback when downloading Ollama models, including percentage complete, current download speed, and estimated time remaining. * test: add focused test coverage for Ollama download progress feature Add unit tests for the critical paths of the real-time download progress tracking: - Progress calculation tests (52 tests): Speed/time/percentage calculations with comprehensive edge case coverage (zero speeds, NaN, Infinity, large numbers) - NDJSON parser tests (33 tests): Streaming JSON parsing from Ollama, buffer management for incomplete lines, error handling All 562 unit tests passing with clean dependencies. Tests focus on critical mathematical logic and data processing - the most important paths that need verification. Test coverage: ✅ Speed calculation and formatting (B/s, KB/s, MB/s) ✅ Time remaining calculations (seconds, minutes, hours) ✅ Percentage clamping (0-100%) ✅ NDJSON streaming with partial line buffering ✅ Invalid JSON handling ✅ Real Ollama API responses ✅ Multi-chunk streaming scenarios * docs: add comprehensive JSDoc docstrings for Ollama download progress feature - Enhanced OllamaModelSelector component with detailed JSDoc * Documented component props, behavior, and usage examples * Added docstrings to internal functions (checkInstalledModels, handleDownload, handleSelect) * Explained progress tracking algorithm and useRef usage - Improved memory-handlers.ts documentation * Added docstring to main registerMemoryHandlers function * Documented all Ollama-related IPC handlers (check-status, list-embedding-models, pull-model) * Added JSDoc to executeOllamaDetector helper function * Documented interface types (OllamaStatus, OllamaModel, OllamaEmbeddingModel, OllamaPullResult) * Explained NDJSON parsing and progress event structure - Enhanced test file documentation * Added docstrings to NDJSON parser test utilities with algorithm explanation * Documented all calculation functions (speed, time, percentage) * Added detailed comments on formatting and bounds-checking logic - Improved overall code maintainability * Docstring coverage now meets 80%+ threshold for code review * Clear explanation of progress tracking implementation details * Better context for future maintainers working with download streaming * feat: add batch task creation and management CLI commands - Handle batch task creation from JSON files - Show status of all specs in project - Cleanup tool for completed specs - Full integration with new apps/backend structure - Compatible with implementation_plan.json workflow * test: add batch task test file and testing checklist - batch_test.json: Sample tasks for testing batch creation - TESTING_CHECKLIST.md: Comprehensive testing guide for Ollama and batch tasks - Includes UI testing steps, CLI testing steps, and edge cases - Ready for manual and automated testing * chore: update package-lock.json to match v2.7.2 * test: update checklist with verification results and architecture validation * docs: add comprehensive implementation summary for Ollama + Batch features * docs: add comprehensive Phase 2 testing guide with checklists and procedures * docs: add NEXT_STEPS guide for Phase 2 testing * fix: resolve merge conflict in project-api.ts from Ollama feature cherry-pick * fix: remove duplicate Ollama check status handler registration * test: update checklist with Phase 2 bug findings and fixes --------- Co-authored-by: ray <[email protected]> * fix: resolve Python environment race condition (#142) Implemented promise queue pattern in PythonEnvManager to handle concurrent initialization requests. Previously, multiple simultaneous requests (e.g., startup + merge) would fail with "Already initializing" error. Also fixed parsePythonCommand() to handle file paths with spaces by checking file existence before splitting on whitespace. Changes: - Added initializationPromise field to queue concurrent requests - Split initialize() into public and private _doInitialize() - Enhanced parsePythonCommand() with existsSync() check Co-authored-by: Joris Slagter <[email protected]> * fix: remove legacy path from auto-claude source detection (#148) Removes the legacy 'auto-claude' path from the possiblePaths array in agent-process.ts. This path was from before the monorepo restructure (v2.7.2) and is no longer needed. The legacy path was causing spec_runner.py to be looked up at the wrong location: - OLD (wrong): /path/to/auto-claude/auto-claude/runners/spec_runner.py - NEW (correct): /path/to/apps/backend/runners/spec_runner.py This aligns with the new monorepo structure where all backend code lives in apps/backend/. Fixes #147 Co-authored-by: Joris Slagter <[email protected]> * Fix/linear 400 error * fix: Linear API authentication and GraphQL types - Remove Bearer prefix from Authorization header (Linear API keys are sent directly) - Change GraphQL variable types from String! to ID! for teamId and issue IDs - Improve error handling to show detailed Linear API error messages 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: Radix Select empty value error in Linear import modal Use '__all__' sentinel value instead of empty string for "All projects" option, as Radix Select does not allow empty string values. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * feat: add CodeRabbit configuration file Introduce a new .coderabbit.yaml file to configure CodeRabbit settings, including review profiles, automatic review options, path filters, and specific instructions for different file types. This enhances the code review process by providing tailored guidelines for Python, TypeScript, and test files. * fix: correct GraphQL types for Linear team queries Linear API uses different types for different queries: - team(id:) expects String! - issues(filter: { team: { id: { eq: } } }) expects ID! 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: refresh task list after Linear import Call loadTasks() after successful Linear import to update the kanban board without requiring a page reload. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * cleanup * cleanup * fix: address CodeRabbit review comments for Linear integration - Fix unsafe JSON parsing: check response.ok before parsing JSON to handle non-JSON error responses (e.g., 503 from proxy) gracefully - Use ID! type instead of String! for teamId in LINEAR_GET_PROJECTS query for GraphQL type consistency - Remove debug console.log (ESLint config only allows warn/error) - Refresh task list on partial import success (imported > 0) instead of requiring full success - Fix pre-existing TypeScript and lint issues blocking commit 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * version sync logic * lints for develop branch * chore: update CI workflow to include develop branch - Modified the CI configuration to trigger on pushes and pull requests to both main and develop branches, enhancing the workflow for development and integration processes. * fix: update project directory auto-detection for apps/backend structure The project directory auto-detection was checking for the old `auto-claude/` directory name but needed to check for `apps/backend/`. When running from `apps/backend/`, the directory name is `backend` not `auto-claude`, so the check would fail and `project_dir` would incorrectly remain as `apps/backend/` instead of resolving to the project root (2 levels up). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: use GraphQL variables instead of string interpolation in LINEAR_GET_ISSUES Replace direct string interpolation of teamId and linearProjectId with proper GraphQL variables. This prevents potential query syntax errors if IDs contain special characters like double quotes, and aligns with the variable-based approach used elsewhere in the file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(ui): correct logging level and await loadTasks on import complete - Change console.warn to console.log for import success messages (warn is incorrect severity for normal completion) - Make onImportComplete callback async and await loadTasks() to prevent potential unhandled promise rejections Applies CodeRabbit review feedback across 3 LinearTaskImportModal usages. * fix(hooks): use POSIX-compliant find instead of bash glob The pre-commit hook uses #!/bin/sh but had bash-specific ** glob pattern for staging ruff-formatted files. The ** pattern only works in bash with globstar enabled - in POSIX sh it expands literally and won't match subdirectories, causing formatted files in nested directories to not be staged. --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * fix(task): stop running process when task status changes away from in_progress When a user drags a running task back to Planning (or any other column), the process was not being stopped, leaving a "ghost" process that prevented deletion with "Cannot delete a running task" error. Now the task process is automatically killed when status changes away from in_progress, ensuring the process state stays in sync with the UI. * feat: Add UI scale feature with 75-200% range (#125) * feat: add UI scale feature * refactor: extract UI scale bounds to shared constants * fix: duplicated import * fix: hide status badge when execution phase badge is showing (#154) * fix: analyzer Python compatibility and settings integration Fixes project index analyzer failing with TypeError on Python type hints. Changes: - Added 'from __future__ import annotations' to all analysis modules - Fixed project discovery to support new analyzer JSON format - Read Python path directly from settings.json instead of pythonEnvManager - Added stderr/stdout logging for analyzer debugging Resolves 'Discovered 0 files' and 'TypeError: unsupported operand type' issues. * auto-claude: subtask-1-1 - Hide status badge when execution phase badge is showing When a task has an active execution (planning, coding, etc.), the execution phase badge already displays the correct state with a spinner. The status badge was also rendering, causing duplicate/confusing badges (e.g., both "Planning" and "Pending" showing at the same time). This fix wraps the status badge in a conditional that only renders when there's no active execution, eliminating the redundant badge display. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(ipc): remove unused pythonEnvManager parameter and fix ES6 import Address CodeRabbit review feedback: - Remove unused pythonEnvManager parameter from registerProjectContextHandlers and registerContextHandlers (the code reads Python path directly from settings.json instead) - Replace require('electron').app with proper ES6 import for consistency 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * chore(lint): fix import sorting in analysis module Run ruff --fix to resolve I001 lint errors after merging develop. All 23 files in apps/backend/analysis/ now have properly sorted imports. --------- Co-authored-by: Joris Slagter <[email protected]> Co-authored-by: Claude Opus 4.5 <[email protected]> * fix/PRs from old main setup to apps structure (#185) * fix(core): add task persistence, terminal handling, and HTTP 300 fixes Consolidated bug fixes from PRs #168, #170, #171: - Task persistence (#168): Scan worktrees for tasks on app restart to prevent loss of in-progress work and wasted API credits. Tasks in .worktrees/*/specs are now loaded and deduplicated with main. - Terminal buttons (#170): Fix "Open Terminal" buttons silently failing on macOS by properly awaiting createTerminal() Promise. Added useTerminalHandler hook with loading states and error display. - HTTP 300 errors (#171): Handle branch/tag name collisions that cause update failures. Added validation script to prevent conflicts before releases and user-friendly error messages with manual download links. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(platform): add path resolution, spaces handling, and XDG support This commit consolidates multiple bug fixes from community PRs: - PR #187: Path resolution fix - Update path detection to find apps/backend instead of legacy auto-claude directory after v2.7.2 restructure - PR #182/#155: Python path spaces fix - Improve parsePythonCommand() to handle quoted paths and paths containing spaces without splitting - PR #161: Ollama detection fix - Add new apps structure paths for ollama_model_detector.py script discovery - PR #160: AppImage support - Add XDG Base Directory compliant paths for Linux sandboxed environments (AppImage, Flatpak, Snap). New files: - config-paths.ts: XDG path utilities - fs-utils.ts: Filesystem utilities with fallback support - PR #159: gh CLI PATH fix - Add getAugmentedEnv() utility to include common binary locations (Homebrew, snap, local) in PATH for child processes. Fixes gh CLI not found when app launched from Finder/Dock. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: address CodeRabbit/Cursor review comments on PR #185 Fixes from code review: - http-client.ts: Use GITHUB_CONFIG instead of hardcoded owner in HTTP 300 error message - validate-release.js: Fix substring matching bug in branch detection that could cause false positives (e.g., v2.7 matching v2.7.2) - bump-version.js: Remove unnecessary try-catch wrapper (exec() already exits on failure) - execution-handlers.ts: Capture original subtask status before mutation for accurate logging - fs-utils.ts: Add error handling to safeWriteFile with proper logging Dismissed as trivial/not applicable: - config-paths.ts: Exhaustive switch check (over-engineering) - env-utils.ts: PATH priority documentation (existing comments sufficient) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: address additional CodeRabbit review comments (round 2) Fixes from second round of code review: - fs-utils.ts: Wrap test file cleanup in try-catch for Windows file locking - fs-utils.ts: Add error handling to safeReadFile for consistency with safeWriteFile - http-client.ts: Use GITHUB_CONFIG in fetchJson (missed in first round) - validate-release.js: Exclude symbolic refs (origin/HEAD -> origin/main) from branch check - python-detector.ts: Return cleanPath instead of pythonPath for empty input edge case Dismissed as trivial/not applicable: - execution-handlers.ts: Redundant checkSubtasksCompletion call (micro-optimization) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> * feat/beta-release (#190) * chore: update README version to 2.7.1 Updated the version badge and download links in the README to reflect t…
…ling (AndyMik90#839) * fix(ACS-175): Resolve integrations freeze and improve rate limit handling * fix(i18n): replace hardcoded toast strings with translation keys Addresses CodeRabbit review feedback on PR AndyMik90#839: - OAuthStep.tsx: use t() for all toast messages - RateLimitModal.tsx: use t() for toast messages - SDKRateLimitModal.tsx: add useTranslation hook, use t() for toasts - Add toast translation keys to en/fr onboarding.json and common.json * fix: replace console.log with debugLog in IntegrationSettings Addresses CodeRabbit review feedback - use project's debug logging utility for consistent and toggle-able logging in production. * fix: replace console.log with debugLog in main process files Addresses Auto-Claude PR Review feedback: - terminal-handlers.ts: 14 console.log → debugLog - pty-manager.ts: 10 console.log → debugLog/debugError - terminal-manager.ts: 4 console.log → debugLog/debugError Also fixes: - Extract magic numbers to CHUNKED_WRITE_THRESHOLD/CHUNK_SIZE constants - Add terminal validity check before chunked writes - Consistent error handling (no rethrow for fire-and-forget semantics) * fix: address Auto Claude PR review findings - console.error→debugError + i18n - Replace 8 remaining console.error/warn calls with debugError/debugLog: - terminal-handlers.ts: lines 59, 426, 660, 671 - terminal-manager.ts: lines 88, 320 - pty-manager.ts: lines 88, 141 - Fixed duplicate logging in exception handler - Add comprehensive i18n for SDKRateLimitModal.tsx (~20 strings): - Added rateLimit.sdk.* keys with swap notifications, buttons, labels - EN + FR translations in common.json - Add comprehensive i18n for OAuthStep.tsx (~15 strings): - Added oauth.badges.*, oauth.buttons.*, oauth.labels.* keys - EN + FR translations in onboarding.json All MEDIUM severity findings resolved except race condition (deferred). * fix: address Auto Claude PR review findings - race condition + console.error - Fix race condition in chunked PTY writes by serializing writes per terminal using Promise chaining (prevents interleaving of concurrent large writes) - Fix missing 't' in useEffect dependency array in OAuthStep.tsx - Convert all remaining console.error calls to debugError for consistency: - IntegrationSettings.tsx (9 occurrences) - RateLimitModal.tsx (3 occurrences) - SDKRateLimitModal.tsx (4 occurrences) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: Black Circle Sentinel <[email protected]> --------- Signed-off-by: Black Circle Sentinel <[email protected]> Co-authored-by: Andy <[email protected]> Co-authored-by: Adam Slaker <[email protected]> Co-authored-by: Claude Opus 4.5 <[email protected]>
5 participants
Summary
Fixes ACS-175. Resolves UI freeze issues during Claude profile integration setup and improves rate limit modal handling for a smoother onboarding experience.
Problem
alert()dialogs disrupt user experience during rate limit handling and authentication flowsSolution
alert()dialogs with modern toast notificationsdebugLogcalls for better troubleshootingChanges
terminal-handlers.tspty-manager.tsterminal-manager.tsRateLimitModal.tsxalert()withuseToast()notificationsSDKRateLimitModal.tsxuseTranslation, replacedalert()with i18n toastsOAuthStep.tsxIntegrationSettings.tsxconsole.logwithdebugLogutilityen/settings.jsonfr/settings.jsonen/common.jsonfr/common.jsonen/onboarding.jsonfr/onboarding.jsonCode Review Fixes Applied
Addressed all CodeRabbit feedback:
OAuthStep.tsxt('oauth.toast.*')i18n callsRateLimitModal.tsxt('rateLimit.toast.*')callsSDKRateLimitModal.tsxuseTranslationhook + i18n toast callsconsole.loginIntegrationSettings.tsxdebugLogfrom debug-logger utilityTest Plan
debugLoginstead ofconsole.log🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Localization
✏️ Tip: You can customize this high-level summary in your review settings.